Feature/systemd query events #1728
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant updates across multiple files, primarily focusing on enhancing DNS management and integration with Windows-specific functionalities. Key changes include the addition of new dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub
participant CI/CD
participant BuildScript
participant WindowsDLL
User->>GitHub: Push changes
GitHub->>CI/CD: Trigger build workflow
CI/CD->>BuildScript: Run build script
BuildScript->>WindowsDLL: Compile DLL
WindowsDLL-->>BuildScript: Return build status
BuildScript-->>CI/CD: Report build status
CI/CD-->>GitHub: Update pull request status
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (15)
service/firewall/bypassing.go (2)
49-49: Consider strengthening the system resolver check.The TODO comment suggests uncertainty about the specificity of the check. Consider adding additional validation such as verifying the process path or using a more comprehensive identification method.
46-56: Document the resolver disabled behavior.Given that this introduces significant new behavior when the resolver is disabled, consider adding a function-level comment explaining:
- When and why the resolver might be disabled
- Security implications of the disabled state
- Interaction with systemd-resolver
service/firewall/interception/dnslistener/varlinktypes.go (3)
1-4: Enhance package documentation.Consider adding more detailed documentation about:
- The varlink protocol and its role in systemd-resolver
- The relationship between these types and the DNS event handling
- Any relevant systemd-resolver documentation references
package dnslistener -// List of struct that define the systemd-resolver varlink dns event protocol. +// Package dnslistener provides types for interacting with systemd-resolver's varlink DNS event protocol. +// These types facilitate the communication between the application and systemd-resolver for DNS query events. +// +// For more information about the protocol, see: +// https://www.freedesktop.org/software/systemd/man/org.freedesktop.resolve1.html
5-9: Add field documentation and consider using DNS-specific types.The struct would benefit from:
- Documentation explaining the purpose of each field
- Using DNS-specific constants for Class and Type fields
+// ResourceKey represents a unique identifier for a DNS resource record. type ResourceKey struct { + // Class represents the DNS class (e.g., IN for Internet) Class int `json:"class"` + // Type represents the DNS record type (e.g., A, AAAA, MX) Type int `json:"type"` + // Name is the domain name for this resource Name string `json:"name"` }Consider defining constants for common DNS classes and types:
// DNS Classes const ( ClassIN = 1 // Internet ClassCS = 2 // CSNET ClassCH = 3 // CHAOS ClassHS = 4 // Hesiod ) // DNS Types const ( TypeA = 1 // IPv4 address TypeNS = 2 // Nameserver TypeCNAME = 5 // Canonical name TypeSOA = 6 // Start of authority TypePTR = 12 // Pointer TypeMX = 15 // Mail exchange TypeTXT = 16 // Text TypeAAAA = 28 // IPv6 address )
65-69: Add struct and field documentation.The struct would benefit from clear documentation explaining its purpose and the significance of each field.
+// Answer represents a DNS response containing either a parsed resource record +// or the raw DNS response data. type Answer struct { + // RR is the parsed DNS resource record, if available RR *ResourceRecord `json:"rr,omitempty"` + // Raw contains the unparsed DNS response string Raw string `json:"raw"` + // IfIndex is the network interface index where this answer was received IfIndex *int `json:"ifindex,omitempty"` }service/compat/module.go (2)
102-106: Consider standardizing skip handling witherrSelfcheckSkipped.The early return for disabled resolver is logically sound, but for consistency with other skip conditions (like the one handled by
errSelfcheckSkipped), consider using the same error pattern.res := module.instance.Resolver() if res.IsDisabled.IsSet() { - log.Debugf("compat: skipping self-check: resolver is disabled") - return nil + return nil, fmt.Errorf("%w: resolver is disabled", errSelfcheckSkipped) }
192-192: LGTM! Consider documenting interface changes.The new
Resolver()method is a good addition for accessing resolver state. Consider adding interface documentation to explain the contract, especially regarding initialization guarantees and thread safety requirements.go.mod (1)
Line range hint
5-6: Track temporary winres dependency replacement.The TODO comment indicates this is a temporary fix waiting for upstream PR #4. Consider:
- Adding a GitHub issue to track the removal of this replacement
- Adding a link to the upstream PR for better context
Would you like me to create a GitHub issue to track this?
service/resolver/main.go (2)
32-33: Add documentation for the exported field.Since
IsDisabledis an exported field that other packages may use, please add a documentation comment explaining its purpose, when it's set, and how it should be used.+// IsDisabled indicates whether the resolver module is currently disabled. +// This is typically set when no resolvers are configured. IsDisabled abool.AtomicBool
32-33: Consider reflecting the disabled state in the state manager.The module uses
mgr.StateMgrfor state management, but the newIsDisabledstate is not reflected there. Consider adding the disabled state to the state manager to maintain consistency and enable proper monitoring.This could be done in the
loadResolversfunction (whereIsDisabledis set) by adding:if module.IsDisabled.IsSet() { module.states.Add("resolver:disabled", "The resolver module is currently disabled as no resolvers are configured") } else { module.states.Remove("resolver:disabled") }Also applies to: 272-274
service/instance.go (2)
192-195: Fix error message for consistencyThe error message uses "dns-listener" with a hyphen, while other module names in error messages don't use hyphens. Consider changing for consistency:
- return instance, fmt.Errorf("create dns-listener module: %w", err) + return instance, fmt.Errorf("create dnslistener module: %w", err)
473-476: Fix comment style for consistencyThe comment uses "dns-listener" with a hyphen, while other module names in comments don't use hyphens. Consider changing for consistency:
-// DNSListener returns the dns-listener module. +// DNSListener returns the dnslistener module.service/firewall/packet_handler.go (1)
Line range hint
447-459: Security review of DNS redirection logic.The implementation maintains robust security controls while adding flexibility:
- Multiple validation layers (resolver status, nameserver IP, port checks)
- Special handling for broadcast queries
- Proper separation between system and user DNS queries
Consider adding debug logging when DNS redirection is skipped due to disabled resolver to aid in troubleshooting.
Add debug logging:
case dnsQueryInterception() && module.instance.Resolver().IsDisabled.IsNotSet() && pkt.IsOutbound() && + // Log when redirection is skipped due to disabled resolver + func() bool { + if module.instance.Resolver().IsDisabled.IsSet() { + log.Tracer(pkt.Ctx()).Debugf("filter: skipping DNS redirection, resolver is disabled: %s", pkt) + } + return true + }() && pkt.Info().DstPort == 53 &&service/firewall/interception/dnslistener/module.go (2)
9-14: Organize imports according togcirecommendationsThe imports are not properly organized as indicated by the
gcistatic analysis tool. Consider runninggciwith--skip-generated -s standard -s defaultto sort and group imports according to Go standards.🧰 Tools
🪛 GitHub Check: Linter
[failure] 9-9:
File is notgci-ed with --skip-generated -s standard -s default (gci)
[failure] 14-14:
File is notgci-ed with --skip-generated -s standard -s default (gci)
144-146: Handle error frominfo.Save()appropriatelyCurrently, when
info.Save()returns an error, it is logged but not returned or handled further. Consider whether this error should be propagated or if additional error handling is necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.mod(2 hunks)service/compat/module.go(3 hunks)service/firewall/bypassing.go(1 hunks)service/firewall/interception/dnslistener/module.go(1 hunks)service/firewall/interception/dnslistener/varlinktypes.go(1 hunks)service/firewall/module.go(3 hunks)service/firewall/packet_handler.go(1 hunks)service/instance.go(5 hunks)service/resolver/main.go(2 hunks)service/resolver/resolvers.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Linter
service/firewall/interception/dnslistener/module.go
[failure] 9-9:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 14-14:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 48-48:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 51-51:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
[failure] 60-60:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 63-63:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
🔇 Additional comments (13)
service/firewall/bypassing.go (2)
46-56: Verify interaction with secure DNS bypass reporting.
The new exceptions for disabled resolver state are placed before the secure DNS filter list checks. This means connections that would normally trigger secure DNS bypass reporting might be allowed when the resolver is disabled.
Let's check the interaction:
#!/bin/bash
# Description: Verify how secure DNS bypass reporting works with disabled resolver
# Test: Check for any conditions or checks around secure DNS bypass reporting
rg "ReportSecureDNSBypassIssue"46-56: Verify security implications of bypass exceptions.
The new conditional block creates exceptions to bypass prevention when the Portmaster resolver is disabled. While the intent is clear, we should ensure these exceptions don't create security vulnerabilities.
Let's verify the impact and usage:
✅ Verification successful
The bypass exceptions are properly implemented with adequate security measures
The verification reveals that the bypass exceptions are well-integrated into the system's security architecture:
-
The system has comprehensive DNS bypass prevention:
- Bypass attempts trigger security notifications via
ReportSecureDNSBypassIssue - There's a global setting
Block Secure DNS Bypassingthat's enabled by default - The firewall maintains protection even when bypassing is attempted
- Bypass attempts trigger security notifications via
-
The localhost and system resolver exceptions are specifically handled:
- System resolver checks are consistently implemented across the codebase
- Localhost scope checks are used in multiple security contexts with proper validation
- The exceptions only apply when the Portmaster resolver is explicitly disabled
-
The conditional block maintains security by:
- Only allowing necessary traffic when the resolver is disabled
- Properly identifying system resolver processes
- Limiting localhost exceptions to DNS port 53
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other security measures or validations when resolver is disabled
# Test 1: Check for any additional security checks or validations when resolver is disabled
rg -A 5 "IsDisabled.IsSet\(\)"
# Test 2: Check how localhost connections are handled elsewhere
rg -A 5 "IsLocalhost\(\)"
Length of output: 400
Script:
#!/bin/bash
# Let's try a more comprehensive verification approach
# Check for IsDisabled usage with proper quoting
rg -A 5 "IsDisabled\.IsSet"
# Check how localhost connections are validated
rg -A 5 "IsLocalhost"
# Check for system resolver related checks
rg -A 5 "IsSystemResolver"
# Check for other DNS bypass prevention mechanisms
rg -A 5 "DNS.*bypass" -i
# Look for related security validations
rg -A 5 "ReportSecureDNSBypassIssue"
Length of output: 13343
service/firewall/interception/dnslistener/varlinktypes.go (1)
71-79: Add documentation and validate pointer usage.
The struct needs documentation, and some fields might not need to be pointers.
+// QueryResult represents the complete result of a DNS query from systemd-resolver,
+// including the query state, any errors, and the received answers.
type QueryResult struct {
+ // Ready indicates if the query has completed
Ready *bool `json:"ready,omitempty"`
+ // State describes the current state of the query
State *string `json:"state,omitempty"`
+ // Rcode is the DNS response code
Rcode *int `json:"rcode,omitempty"`
+ // Errno is the system error number, if any
Errno *int `json:"errno,omitempty"`
+ // Question contains the original DNS query
Question *[]ResourceKey `json:"question,omitempty"`
+ // CollectedQuestions contains additional related queries
CollectedQuestions *[]ResourceKey `json:"collectedQuestions,omitempty"`
+ // Answer contains the DNS responses
Answer *[]Answer `json:"answer,omitempty"`
}Consider whether some fields really need to be pointers. For example:
Readycould be a non-pointerboolwith a default value offalseStatecould be an enum type instead of a string pointerRcodeandErrnocould be non-pointer integers with sentinel values (-1) for "not set"
service/firewall/module.go (1)
19-19: LGTM: Clean integration of resolver module access.
The addition of the resolver module access follows the established pattern of module getter methods in the interface. This change maintains consistency with the existing architecture.
Let's verify that all implementations of the instance interface have been updated:
Also applies to: 168-168
✅ Verification successful
Let me gather more information to verify the interface implementation comprehensively.
LGTM: The resolver module integration is properly implemented.
The verification confirms that:
- The
Instancestruct is the only implementation of theinstanceinterface - The
Instancestruct has both the required fieldresolver *resolver.ResolverModuleand the methodResolver() *resolver.ResolverModule - All interface methods are implemented by the
Instancestruct
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential implementations of the instance interface
# to ensure they've been updated with the new Resolver() method.
# Search for struct types that might implement the interface
ast-grep --pattern 'type $name struct {
$$$
}'
# Search for interface method implementations
rg -A 1 'func \([^)]+\) Resolver\(\)'
Length of output: 206
Script:
#!/bin/bash
# First, let's find the interface definition to understand all required methods
rg -A 10 "type instance interface" service/firewall/module.go
# Then find all structs that could implement this interface
rg "type.*struct" service/instance.go
# Finally, check if Instance is the only implementation
fd -e go | xargs rg "type.*struct.*{"
Length of output: 496
Script:
#!/bin/bash
# Let's fix the regex and search for struct implementations
fd -e go | xargs rg "type.*struct"
# Also check if Instance implements all interface methods
rg "func \(i \*Instance\)" service/instance.go
# And verify the resolver field exists in Instance
rg "resolver.*\*resolver\.ResolverModule" service/instance.go
Length of output: 40121
service/compat/module.go (1)
127-128:
Potential race condition in resolver state handling.
The early return (lines 102-106) should prevent execution from reaching this point when the resolver is disabled. The fact that we're checking again suggests the resolver state could change during self-check execution. Consider:
- Adding atomic operations or proper synchronization
- Removing this redundant check
- Adding a comment explaining why both checks are necessary
Let's verify if the resolver state can change during execution:
go.mod (2)
37-37: Document new dependency purposes.
Two new direct dependencies have been added:
github.com/maruel/panicparse/v2: Consider documenting why this debugging tool is neededgithub.com/varlink/go: Required for systemd-resolver DNS event protocol integration
#!/bin/bash
# Check for usage of new dependencies in codebase
echo "Checking panicparse usage..."
rg "panicparse" --type go
echo "Checking varlink usage..."
rg "varlink" --type goConsider adding comments in the code or documentation explaining the purpose of these dependencies.
Also applies to: 61-61
Line range hint 3-3: Verify Go 1.22.0 compatibility requirements.
Go 1.22.0 is a very recent version (released Feb 2024). Please ensure all deployment environments and CI/CD pipelines support this version.
service/instance.go (4)
22-22: LGTM!
The import is correctly placed and follows the project's import grouping convention.
78-78: LGTM!
The dnslistener field is correctly placed and follows the established pattern for module fields.
297-297: LGTM!
The dnslistener module is correctly registered in the service group, maintaining the logical grouping with other interception-related modules.
78-78: Verify SleepyModule interface implementation
Since this is a new module in the service group, verify if it needs to implement the SleepyModule interface like other modules. This interface is used by the SetSleep method to manage module sleep states.
Also applies to: 192-195, 297-297, 473-476
service/resolver/resolvers.go (1)
401-408: 🛠️ Refactor suggestion
Consider moving the disabled state management after resolver validation.
The current implementation sets the module's disabled state before validating resolvers and potentially loading defaults. This could lead to incorrect state representation if:
- The configured resolvers are invalid and defaults are loaded
- Other modules check the disabled state while resolvers are being loaded
Consider moving this block after the resolver validation and default loading (after line 440) to ensure the state accurately reflects the final resolver configuration:
- // If no resolvers are configure set the disabled state. So other modules knows that the users does not want to use Portmaster resolver.
- if len(newResolverConfig) == 0 {
- module.IsDisabled.Set()
- } else {
- module.IsDisabled.UnSet()
- }
currentResolverConfig = newResolverConfig
newResolvers := append(
getConfiguredResolvers(newResolverConfig),
getSystemResolvers()...,
)
if len(newResolvers) == 0 {
// load defaults directly, overriding config system
newResolvers = getConfiguredResolvers(defaultNameServers)
...
}
+
+ // Set the disabled state after resolver validation and default loading
+ if len(newResolvers) == 0 {
+ module.IsDisabled.Set()
+ } else {
+ module.IsDisabled.UnSet()
+ }Let's verify the impact of this state management on other modules:
service/firewall/packet_handler.go (1)
447-449: LGTM! Verify DNS redirection behavior.
The additional check for resolver status is correctly implemented, ensuring DNS queries are only redirected when the resolver is enabled.
Let's verify the resolver status handling:
✅ Verification successful
Let me gather more information about the DNS redirection logic to ensure a comprehensive verification.
DNS redirection logic is correctly implemented with resolver status check
The verification confirms that:
- The resolver status check
IsDisabled.IsNotSet()is consistently used across the codebase for DNS handling - The DNS redirection logic is properly integrated with the resolver status:
- In packet handling (
service/firewall/packet_handler.go) - In bypassing logic (
service/firewall/bypassing.go) - In compatibility checks (
service/compat/module.go)
- In packet handling (
- The implementation correctly ensures DNS queries are only redirected when:
- The resolver is enabled (IsDisabled.IsNotSet())
- The packet is outbound
- The destination port is 53
- The destination is not our nameserver
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent resolver status handling across the codebase
# Test: Search for other DNS redirection logic that might need similar checks
rg -A 5 "DstPort == 53"
# Test: Search for other resolver status checks to ensure consistent patterns
ast-grep --pattern 'Resolver().IsDisabled'
Length of output: 895
Script:
#!/bin/bash
# Search for DNS query interception function and its usage
ast-grep --pattern 'dnsQueryInterception()'
# Search for resolver status checks in a broader context
rg -A 5 "IsDisabled"
# Look for nameserver related functions to understand DNS handling
rg -A 5 "RerouteToNameserver"
Length of output: 11331
service/resolver/main.go
Outdated
| mgr: m, | ||
| instance: instance, | ||
| IsDisabled: *abool.New(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary dereferencing of abool.New().
The abool.New() function returns a pointer to AtomicBool, but the field type is not a pointer. The dereferencing is unnecessary and could be error-prone.
- IsDisabled: *abool.New(),
+ IsDisabled: abool.New(),📝 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.
| mgr: m, | |
| instance: instance, | |
| IsDisabled: *abool.New(), | |
| mgr: m, | |
| instance: instance, | |
| IsDisabled: abool.New(), |
| if err != nil { | ||
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | ||
| return nil |
There was a problem hiding this comment.
Return error instead of nil on connection failure
In the Start() method, when failing to establish a varlink connection, the error is logged but nil is returned. Consider returning the error to allow the caller to handle it appropriately.
Apply this diff to fix the issue:
if err != nil {
log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err)
- return nil
+ return err
}📝 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.
| if err != nil { | |
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | |
| return nil | |
| if err != nil { | |
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | |
| return err |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (10)
service/network/connection.go (1)
552-552: Consider handling the Flush error.The error from
DNSListener().Flush()is being silently ignored. While this might be intentional, it's worth logging the error for debugging purposes.- _ = module.instance.DNSListener().Flush() + if err := module.instance.DNSListener().Flush(); err != nil { + log.Tracer(pkt.Ctx()).Debugf("network: failed to flush dns listener: %v", err) + }service/firewall/interception/dnslistener/eventlistener_linux.go (4)
86-86: Correct typos in comments for better readabilityThere are typographical errors in the comments that should be corrected:
- Line 86: "Allocated data struct for the parsed result." should be "Allocate data structures for the parsed result."
- Line 97: "Go trough each answer entry." should be "Go through each answer entry."
Making these corrections enhances the readability and professionalism of the code.
Also applies to: 97-97
143-145: Consider additional error handling when saving IP info recordsWhen
info.Save()fails, the error is logged but not otherwise handled. Depending on the importance of this operation, additional actions might be necessary.Consider implementing retry logic or propagating the error to ensure the issue is addressed appropriately.
if err := info.Save(); err != nil { log.Errorf("nameserver: failed to save IP info record: %s", err) + // Additional error handling here }
78-83: SetvarlinkConntonilafter closingAfter closing the
varlinkConn, it's a good practice to set it tonilto prevent potential nil-pointer dereferences or misuse after closure.Apply this diff to update the
stopmethod:func (l *Listener) stop() error { if l.varlinkConn != nil { err := l.varlinkConn.Close() + l.varlinkConn = nil return err } return nil }
25-27: Improve error handling when Varlink service is unavailableIf the Varlink connection to the systemd resolver cannot be established, the function returns an error immediately.
Consider implementing a retry mechanism or handling the error gracefully to improve resilience against temporary service unavailability.
if err != nil { - return nil, fmt.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %w", err) + log.Warnf("dnslistener: failed to connect, retrying: %s", err) + // Implement retry logic here }service/firewall/interception/dnslistener/etwlink_windows.go (5)
28-28: Correct typos in the function comment.In the comment for
NewSession, "initilizes" should be "initializes", and "DestorySession" should be "DestroySession". This improves readability and professionalism.Apply this diff to fix the typos:
-// NewSession creates new ETW event listener and initilizes it. This is a low level interface, make sure to call DestorySession when you are done using it. +// NewSession creates a new ETW event listener and initializes it. This is a low-level interface; make sure to call DestroySession when you are done using it.
36-36: Fix typos in error messages when loading DLL functions.There are several instances where "faild" should be "failed" in the error messages. Correcting these typos enhances clarity and professionalism.
Apply this diff to correct the typos:
- return nil, fmt.Errorf("faild to load dll: %q", err) + return nil, fmt.Errorf("failed to load dll: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWCreateState: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWCreateState: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWInitializeSession: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWInitializeSession: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWStartTrace: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWStartTrace: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWFlushTrace: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWFlushTrace: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWStopTrace: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWStopTrace: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWDestroySession: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWDestroySession: %q", err)Also applies to: 40-40, 44-44, 48-48, 52-52, 56-56, 60-60
80-80: Correct the typo in the error message."initialzie" should be "initialize" in the error message for clarity.
Apply this diff to fix the typo:
- return nil, fmt.Errorf("failed to initialzie session: error code: %q", rc) + return nil, fmt.Errorf("failed to initialize session: error code: %q", rc)
112-112: Fix the typo in the function comment.In the comment for
StopTrace, "stopes" should be "stops" to correct the spelling.Apply this diff to correct the typo:
-// StopTrace stopes the trace. This will cause StartTrace to return. +// StopTrace stops the trace. This will cause StartTrace to return.
69-69: Handle potential errors fromcreateState.Call().Even if the function is expected not to fail, it's good practice to check and handle any unexpected errors from
createState.Call(). This can prevent potential issues if assumptions change in the future or under different environments.Consider modifying the code to capture and handle any errors:
- etwListener.state, _, _ = etwListener.createState.Call(win32Callback) + etwListener.state, _, err = etwListener.createState.Call(win32Callback) + if etwListener.state == 0 || err != nil { + return nil, fmt.Errorf("failed to create state: %v", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
service/firewall/interception/dnslistener/etwlink_windows.go(1 hunks)service/firewall/interception/dnslistener/eventlistener.go(1 hunks)service/firewall/interception/dnslistener/eventlistener_linux.go(1 hunks)service/firewall/interception/dnslistener/eventlistener_windows.go(1 hunks)service/firewall/interception/dnslistener/module.go(1 hunks)service/firewall/interception/dnslistener/varlinktypes.go(1 hunks)service/network/connection.go(2 hunks)service/network/module.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- service/firewall/interception/dnslistener/eventlistener.go
🚧 Files skipped from review as they are similar to previous changes (2)
- service/firewall/interception/dnslistener/module.go
- service/firewall/interception/dnslistener/varlinktypes.go
🔇 Additional comments (3)
service/network/module.go (1)
12-12: LGTM: Import additions are appropriate
The new imports for dnslistener and resolver packages align with the interface extensions and PR objectives.
Also applies to: 17-17
service/firewall/interception/dnslistener/eventlistener_linux.go (2)
125-134: Verify modification of domain within the loop
In the loop processing CNAME records, the variable domain is reassigned:
domain = nextDomainThis could affect the outer scope if domain is needed after the loop.
Please verify that changing domain within this loop is intentional and does not impact subsequent code that relies on the original domain value.
121-121:
Undefined variable ResolverInfo
The variable ResolverInfo used here is not defined within the current scope or imported from another package. This will result in a compilation error.
Please define ResolverInfo or import it from the appropriate package. If it's meant to be a reference to an existing resolver, ensure it's correctly initialized and accessible.
Apply this diff if ResolverInfo should reference an existing variable:
+var ResolverInfo = resolver.ResolverInfo{
+ // Initialize with appropriate values
+}Likely invalid or redundant comment.
service/firewall/interception/dnslistener/eventlistener_windows.go
Outdated
Show resolved
Hide resolved
service/firewall/interception/dnslistener/eventlistener_windows.go
Outdated
Show resolved
Hide resolved
service/firewall/interception/dnslistener/eventlistener_windows.go
Outdated
Show resolved
Hide resolved
service/firewall/interception/dnslistener/eventlistener_windows.go
Outdated
Show resolved
Hide resolved
service/firewall/interception/dnslistener/eventlistener_linux.go
Outdated
Show resolved
Hide resolved
service/firewall/interception/dnslistener/eventlistener_linux.go
Outdated
Show resolved
Hide resolved
vlabo
force-pushed
the
feature/systemd-query-events
branch
from
52d9eea to
b3b1615
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (20)
service/integration/integration_windows.go (3)
1-2: Consider using only the new build constraint style.The file uses both old (
// +build windows) and new (//go:build windows) build constraint styles. While this works, it's recommended to standardize on the new style for better maintainability.-//go:build windows -// +build windows +//go:build windows
13-16: Add documentation for the OSSpecific struct.The struct and its fields would benefit from documentation comments explaining their purpose and usage, especially since they handle critical Windows-specific functionality.
+// OSSpecific holds Windows-specific resources and functionality type OSSpecific struct { + // dll holds the loaded portmaster-core.dll instance dll *windows.DLL + // etwFunctions contains the ETW (Event Tracing for Windows) function references etwFunctions ETWFunctions }
41-44: Add documentation about thread safety and usage.The method needs documentation about its thread safety guarantees and expected usage patterns, especially since it's dealing with Windows ETW functionality.
+// GetETWInterface returns the initialized ETW functions. +// This method is thread-safe and can be called from multiple goroutines. +// The returned ETWFunctions should not be stored beyond the lifetime of the OSIntegration instance. func (i *OSIntegration) GetETWInterface() ETWFunctions { return i.os.etwFunctions }service/integration/module.go (2)
11-19: Add documentation for struct fields.While the struct itself is documented, adding comments for each field would improve code maintainability:
m: Purpose of the managerstates: Role of the state manageros: Expected implementation of OSSpecific interfaceinstance: Usage of the instance interface// OSIntegration module provides special integration with the OS. type OSIntegration struct { + // m is the module manager handling the lifecycle m *mgr.Manager + // states manages the module's state transitions states *mgr.StateMgr + // os implements OS-specific integration functionality os OSSpecific + // instance provides access to updates functionality instance instance }
55-57: Document the instance interface.Add documentation explaining the purpose of this interface and its relationship with the updates package.
+// instance provides access to the updates functionality required by the OSIntegration module. type instance interface { Updates() *updates.Updates }service/firewall/interception/dnslistener/eventlistener_windows.go (2)
20-35: Consider enhancing error handling in the goroutine.While the initial error handling is good, the goroutine's error return is not being monitored. Consider adding error handling for
StartTrace()failures in the goroutine.module.mgr.Go("etw-dns-event-listener", func(w *mgr.WorkerCtx) error { - return listener.etw.StartTrace() + if err := listener.etw.StartTrace(); err != nil { + // Log the error or notify through a channel + return fmt.Errorf("failed to start ETW trace: %v", err) + } + return nil })
37-39: Add nil checks for defensive programming.Consider adding nil checks similar to the
stopmethod to prevent potential panic.func (l *Listener) flush() error { + if l == nil { + return fmt.Errorf("listener is nil") + } + if l.etw == nil { + return fmt.Errorf("invalid etw session") + } return l.etw.FlushTrace() }service/firewall/interception/dnslistener/etwlink_windows.go (2)
47-47: Fix typo in error message.There's a typo in "initialzie" which should be "initialize".
- return nil, fmt.Errorf("failed to initialzie session: %q", err) + return nil, fmt.Errorf("failed to initialize session: %q", err)
53-56: Document possible error conditions.The
StartTraceandStopTracemethods should document the possible error conditions that can be returned. This helps users of the API handle errors appropriately.Example documentation:
// StartTrace starts the tracing session of dns events. This is a blocking call. // It will not return until the trace is stopped. // Returns error if the trace session fails to start or if the session is already destroyed. func (l *ETWSession) StartTrace() error {Also applies to: 76-79
service/firewall/interception/dnslistener/module.go (3)
16-20: Add documentation for ResolverInfoConsider adding a doc comment explaining the purpose and usage of this global variable, as it seems to be an important configuration for the systemd resolver integration.
+// ResolverInfo defines the identity and type of the systemd resolver integration. var ResolverInfo = resolver.ResolverInfo{
22-27: Add documentation for DNSListener structThe struct appears to be a core component. Consider adding documentation explaining its purpose and responsibilities.
+// DNSListener manages DNS event listening and processing for systemd resolver integration. type DNSListener struct {
102-119: Add documentation for singleton implementationConsider adding documentation to explain:
- The purpose of shimLoaded and its thread-safety guarantees
- The New function's singleton behavior
- The instance interface requirements
+// shimLoaded ensures only one DNSListener instance is created var shimLoaded atomic.Bool +// New creates a singleton DNSListener instance. +// Returns an error if called multiple times. func New(instance instance) (*DNSListener, error) { +// instance defines the required capabilities for DNSListener integration. type instance interface {service/firewall/interception/dnslistener/eventlistener_linux.go (6)
21-21: Consider making the Varlink socket path configurable.The hardcoded path
/run/systemd/resolve/io.systemd.Resolve.Monitormight not be available on all Linux systems or configurations.Consider making it configurable:
+const defaultVarlinkSocket = "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor" + func newListener(module *DNSListener) (*Listener, error) { - varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor") + socketPath := module.config.GetString("varlink.socket", defaultVarlinkSocket) + varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), socketPath)
69-72: Consider removing unused flush method.The
flushmethod doesn't perform any operation and returns nil. If it's not required by an interface, consider removing it to reduce code clutter.
93-93: Fix typo in comment."Go trough" should be "Go through".
83-84: Document the magic number in slice capacity.The initial capacity of 5 for the IPs slice seems arbitrary. Consider adding a comment explaining why this value was chosen or make it a named constant.
+// typical DNS response rarely contains more than 5 IP addresses ips := make([]net.IP, 0, 5)
95-103: Add nil checks for RR fields.The code assumes that
a.RR.Addressanda.RR.Nameare safe to dereference when non-nil. Consider adding more defensive checks.- if a.RR.Address != nil { + if a.RR != nil && a.RR.Address != nil { ip := net.IP(*a.RR.Address) ips = append(ips, ip) - } else if a.RR.Name != nil { + } else if a.RR != nil && a.RR.Name != nil { cnames[domain] = *a.RR.Name }
106-106: Document the saveDomain function usage.Add a comment explaining what
saveDomaindoes and where it's defined, as it's not immediately clear from this file.+// saveDomain stores the resolved domain information in the DNS cache +// See module.go for implementation saveDomain(domain, ips, cnames)service/integration/etw_windows.go (1)
65-67: Include function names in error messages for clarityIn the error messages returned by methods like
InitializeSession,StartTrace,FlushTrace,StopTrace,DestroySession, andStopOldSession, consider including the function name in the error message to improve clarity during debugging.For example, modify the error messages as follows:
In
InitializeSession:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("InitializeSession failed with status code: %d", rc)In
StartTrace:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("StartTrace failed with status code: %d", rc)In
FlushTrace:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("FlushTrace failed with status code: %d", rc)In
StopTrace:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("StopTrace failed with status code: %d", rc)In
DestroySession:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("DestroySession failed with status code: %d", rc)In
StopOldSession:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("StopOldSession failed with status code: %d", rc)Also applies to: 73-75, 82-84, 91-93, 100-102, 109-111
service/network/connection.go (1)
548-560: Handle errors and add context in the retry mechanismAfter the retry loop, if the
errremains non-nil, consider handling the error or logging it for better debugging. Additionally, providing context with the error messages can aid in troubleshooting.Apply this diff to enhance error handling:
for i := 0; i < 4; i++ { _ = module.instance.DNSListener().Flush() ipinfo, err = resolver.GetIPInfo(resolver.IPInfoProfileScopeGlobal, pkt.Info().RemoteIP().String()) if err == nil { log.Tracer(pkt.Ctx()).Debugf("network: found domain from dnslistener after %d tries", i+1) break } time.Sleep(5 * time.Millisecond) } +if err != nil { + log.Tracer(pkt.Ctx()).Warningf("network: failed to resolve domain after retries: %v", err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
service/firewall/interception/dnslistener/etwlink_windows.go(1 hunks)service/firewall/interception/dnslistener/eventlistener.go(1 hunks)service/firewall/interception/dnslistener/eventlistener_linux.go(1 hunks)service/firewall/interception/dnslistener/eventlistener_windows.go(1 hunks)service/firewall/interception/dnslistener/module.go(1 hunks)service/instance.go(9 hunks)service/integration/etw_windows.go(1 hunks)service/integration/integration.go(1 hunks)service/integration/integration_windows.go(1 hunks)service/integration/module.go(1 hunks)service/network/connection.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- service/integration/integration.go
🚧 Files skipped from review as they are similar to previous changes (1)
- service/firewall/interception/dnslistener/eventlistener.go
🧰 Additional context used
📓 Learnings (1)
service/firewall/interception/dnslistener/etwlink_windows.go (1)
Learnt from: vlabo
PR: safing/portmaster#1728
File: service/firewall/interception/dnslistener/etwlink_windows.go:71-74
Timestamp: 2024-11-18T14:08:51.570Z
Learning: In the file `service/firewall/interception/dnslistener/etwlink_windows.go`, the developer is explicitly calling `DestroySession` and is using `runtime.SetFinalizer` as an additional safety net to enhance resilience in case `DestroySession` is not called due to a panic or error.
🔇 Additional comments (15)
service/integration/integration_windows.go (1)
6-11: LGTM!
The imports are appropriate and necessary for the Windows-specific functionality.
service/integration/module.go (1)
1-9: LGTM! Clean package structure and imports.
The package name and imports are well-organized and appropriate for the module's functionality.
service/firewall/interception/dnslistener/eventlistener_windows.go (4)
1-4: LGTM: Build tags are properly configured.
The build constraints are correctly formatted and include both old and new style tags for maximum compatibility.
6-14: LGTM: Imports are well-organized.
The imports are properly grouped and all are necessary for the implementation.
16-18: Verify ETWSession type definition.
The struct looks good, but let's verify the ETWSession type definition.
✅ Verification successful
Let me try another search strategy to locate the ETWSession type definition.
ETWSession type is properly defined and implemented
The ETWSession type is well-defined in service/firewall/interception/dnslistener/etwlink_windows.go. It's a Windows-specific implementation that includes:
- Integration with ETW functions interface
- Proper shutdown handling with atomic flags and mutex
- Complete lifecycle management (initialization, start, stop, destroy)
- Memory management with runtime finalizer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ETWSession type definition
ast-grep --pattern 'type ETWSession struct {
$$$
}'
# Backup search using ripgrep
rg "type ETWSession"
Length of output: 254
Script:
#!/bin/bash
# Search for ETWSession in the entire codebase
rg "ETWSession" -A 5
# Also search for files containing ETW
fd -e go -e h -e c "ETW"
Length of output: 4683
93-93: Verify saveDomain function definition and error handling.
The saveDomain function is called but not defined in this file. Let's verify its implementation and error handling.
✅ Verification successful
Error handling is properly implemented in saveDomain function
The saveDomain function is defined in module.go and includes appropriate error handling. It:
- Properly checks and skips localhost IPs
- Has error handling for the
Save()operation with logging - Uses appropriate domain record structures with CNAME handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for saveDomain function definition
ast-grep --pattern 'func saveDomain($domain string, $ips []net.IP, $cnames map[string]string) {
$$$
}'
# Backup search using ripgrep
rg "func saveDomain"
Length of output: 254
Script:
#!/bin/bash
# Get the implementation of saveDomain function with context
rg -A 20 "func saveDomain" service/firewall/interception/dnslistener/module.go
Length of output: 633
Script:
#!/bin/bash
# Get more context of the saveDomain function implementation
rg -A 30 "func saveDomain" service/firewall/interception/dnslistener/module.go
# Check if there's any error handling in the module
rg "resolver\.SaveResolvedDomain|resolver\.Save" service/firewall/interception/dnslistener/module.go
Length of output: 941
Script:
#!/bin/bash
# Get the complete implementation including where the record is saved
rg -B 5 -A 40 "func saveDomain" service/firewall/interception/dnslistener/module.go
# Also check for any resolver package usage
rg "resolver\." service/firewall/interception/dnslistener/module.go
Length of output: 1427
service/firewall/interception/dnslistener/etwlink_windows.go (3)
1-20: LGTM! Well-structured type definition with proper synchronization.
The ETWSession struct is well-designed with:
- Thread-safe shutdown guard using atomic operations
- Proper synchronization using mutex
- Clean interface separation for ETW functions
32-35: Ensure the callback function is not garbage collected prematurely.
81-98: Verify ETW session cleanup on process termination.
While the code handles cleanup well during normal operation, we should verify that ETW sessions are properly cleaned up if the process is terminated unexpectedly.
✅ Verification successful
Let me analyze the code further to understand the ETW session cleanup mechanisms.
ETW session cleanup is properly handled
The code has robust cleanup mechanisms in place:
- A runtime finalizer ensures ETW sessions are destroyed even if
DestroySession()isn't explicitly called StopOldSession()is called during initialization to clean up any lingering sessions- The
DestroySession()method is thread-safe with mutex and atomic guards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any leaked ETW sessions from previous runs
# Note: This would require access to Windows ETW APIs which aren't available in the sandbox
# The following is a pseudo-command showing what we'd want to verify:
# Get-EtwTraceSession | Where-Object { $_.LoggerName -like "*portmaster*" }
# Instead, we can look for any cleanup-related code in the codebase
rg -A 5 "StopOldSession|DestroySession" --type go
Length of output: 5455
Script:
#!/bin/bash
# Check for any finalizer or cleanup related code
rg -A 5 "SetFinalizer|runtime\.SetFinalizer" --type go
# Look for any init or startup code that might handle old sessions
rg -A 5 "NewETWSession|InitializeSession" --type go
Length of output: 3174
service/firewall/interception/dnslistener/eventlistener_linux.go (3)
1-14: LGTM! Build tags and imports are properly structured.
The build constraints and imports are well-organized and necessary for the functionality.
15-18: LGTM! Clean and focused struct definition.
The Listener struct is well-defined with a clear purpose.
41-41: Duplicate Comment: Undefined type QueryResult.
This issue was already identified in a previous review. Please ensure the QueryResult type is properly defined or imported.
service/instance.go (1)
70-70: Verify systemd integration compatibility
The addition of integration and dnslistener modules suggests integration with systemd. Please ensure:
- The systemd version compatibility requirements are documented
- The integration gracefully handles systems without systemd
Also applies to: 80-80
✅ Verification successful
Let's search for more concrete evidence of systemd integration and error handling patterns in the codebase. This will help us verify:
- How systemd integration is implemented
- Whether there's proper error handling
- If there's any documentation about systemd requirements
Let's search for more specific evidence of systemd integration implementation and error handling to verify:
- How the code handles systems without systemd
- Whether there's proper error handling for systemd integration
- If there's documentation about systemd requirements
Based on the search results, I can now generate a final response:
Systemd integration is properly handled and platform-specific
The systemd integration is:
- Only used on Linux and properly isolated in platform-specific files (
*_linux.go) - Handles errors appropriately with detailed error messages
- Implements graceful fallbacks through OS-specific interfaces
Key evidence:
- The
OSIntegrationinterface provides platform abstraction - Linux-specific systemd code is isolated in
eventlistener_linux.go - Windows has its own implementation in
integration_windows.go - Error handling includes specific messages for systemd connection failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for systemd version requirements or compatibility documentation
rg -l 'systemd.*version|version.*systemd'
# Check for systemd availability handling
ast-grep --pattern 'if err := systemd.$_(); err != nil {
$$$
}'
Length of output: 111
Script:
#!/bin/bash
# Search for systemd related code and error handling patterns
rg -A 5 'systemd|osintegration' --type go
# Look for any documentation files mentioning systemd
fd -e md -e txt -X rg -l 'systemd'
# Check for integration error handling patterns
ast-grep --pattern 'if $_ == nil {
$$$
}'
Length of output: 118470
Script:
#!/bin/bash
# Search for systemd-related error handling and availability checks
rg -A 5 'systemd.*err|if.*systemd|systemd.*available' --type go
# Look for systemd integration implementation
rg -A 10 'OSIntegration|integration.*systemd' --type go
# Check for any systemd-related documentation
fd -e md -X rg -l 'systemd'
Length of output: 7766
service/network/connection.go (2)
8-8: Ensure the runtime package is necessary
Confirm that the runtime package is required. If it's only used for runtime.GOOS, consider using the standard os package instead.
Run the following script to check the usage of runtime.GOOS:
✅ Verification successful
Let me try a different approach to verify the usage of the runtime package.
Based on the code analysis, I can now provide a final response regarding the usage of the runtime package.
The runtime package is necessary in this file
The runtime package is used for Windows-specific domain resolution handling, which is a critical platform-specific functionality. The usage at line 1012 checks the operating system to implement special handling for delayed domain resolution on Windows systems.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if only `runtime.GOOS` is used from the `runtime` package.
# Search for `runtime.GOOS` usage
rg 'runtime\.GOOS'
Length of output: 1281
Script:
#!/bin/bash
# Search for all runtime package usages in the codebase
rg 'runtime\.' -A 2
Length of output: 95950
Script:
#!/bin/bash
# Check the specific file for runtime usage
cat service/network/connection.go
Length of output: 31106
22-22: Verify the necessity of the reference package import
Ensure that all usages of the reference package are intentional and required.
Run the following script to find references to the reference package:
✅ Verification successful
Let me verify the usage of the reference package in the specific file and its dependencies.
Let me check the actual reference package content to understand its usage.
Let me check the content of these reference package files to understand their purpose and usage.
Based on the analysis of the code, I can now generate the final response.
The reference package import is necessary and correctly used
The reference package provides essential protocol and port mapping utilities that are actively used in connection.go. Specifically:
reference.IsICMP()is used to check if a connection uses ICMP protocol in theshouldWaitForDomain()method- The package provides critical functionality for mapping between protocol numbers and names, port numbers and names
- It's also used by other files in the codebase (
profile/endpoints/endpoint.goandprocess/find.go) for similar protocol and port reference purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of the `reference` package.
# Search for `reference.` usage
rg 'reference\.'
Length of output: 1281
Script:
#!/bin/bash
# Check the content of the connection.go file to understand the context
cat service/network/connection.go
Length of output: 31106
Script:
#!/bin/bash
# Check if there are any other imports of the reference package
rg --type go "\"github.com/safing/portmaster/service/network/reference\""
Length of output: 339
Script:
#!/bin/bash
# Check the reference package itself
fd reference.go -e go
Length of output: 23
Script:
#!/bin/bash
# Find and check the reference package files
rg --type go -l "package reference"
Length of output: 111
Script:
#!/bin/bash
# Search for the actual implementation files in the reference package directory
ls -la service/network/reference/
Length of output: 266
Script:
#!/bin/bash
# Check the content of the reference package files
cat service/network/reference/ports.go service/network/reference/protocols.go
Length of output: 3427
| func (i *OSIntegration) Initialize() error { | ||
| // Find path to the dll. | ||
| file, err := updates.GetFile("portmaster-core.dll") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Load the DLL. | ||
| i.os.dll, err = windows.LoadDLL(file.Path()) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load dll: %q", err) | ||
| } | ||
|
|
||
| // Enumerate all needed dll functions. | ||
| i.os.etwFunctions, err = initializeETW(i.os.dll) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the investigation of the codebase, I can now generate the final response:
Add cleanup handling for DLL loading
The review comment is partially correct. While the DLL path validation is not strictly necessary since it comes from a trusted source (updates package), cleanup handling should be added to prevent resource leaks:
func (i *OSIntegration) Initialize() error {
// Find path to the dll.
file, err := updates.GetFile("portmaster-core.dll")
if err != nil {
return err
}
// Load the DLL.
i.os.dll, err = windows.LoadDLL(file.Path())
if err != nil {
return fmt.Errorf("failed to load dll: %q", err)
}
+ // Ensure cleanup on failure
+ var success bool
+ defer func() {
+ if !success {
+ i.os.dll.Release()
+ i.os.dll = nil
+ }
+ }()
// Enumerate all needed dll functions.
i.os.etwFunctions, err = initializeETW(i.os.dll)
if err != nil {
return err
}
+ success = true
return nil
}The other suggestions from the review are not necessary because:
- The DLL path comes from the updates package which handles file integrity and security
- The updates package already validates and manages file paths internally
🔗 Analysis chain
Add cleanup handling and validate DLL path.
The initialization process has several potential issues that need to be addressed:
- There's no cleanup if initialization fails after the DLL is loaded
- The DLL path should be validated before loading
- Loading a DLL from the updates package has security implications
Here's a suggested improvement:
func (i *OSIntegration) Initialize() error {
// Find path to the dll.
file, err := updates.GetFile("portmaster-core.dll")
if err != nil {
return err
}
+ // Validate DLL path
+ if err := validateDLLPath(file.Path()); err != nil {
+ return fmt.Errorf("invalid DLL path: %w", err)
+ }
+
// Load the DLL.
i.os.dll, err = windows.LoadDLL(file.Path())
if err != nil {
return fmt.Errorf("failed to load dll: %q", err)
}
+ // Ensure cleanup on failure
+ defer func() {
+ if err != nil {
+ i.os.dll.Release()
+ i.os.dll = nil
+ }
+ }()
// Enumerate all needed dll functions.
i.os.etwFunctions, err = initializeETW(i.os.dll)
if err != nil {
return err
}
return nil
}Let's verify the DLL loading security:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check DLL loading security implications
# Check if the DLL is loaded from a secure location
rg -l "updates.GetFile.*portmaster-core.dll"
# Check for any validation of DLL paths
rg -l "validateDLL|DLLPath|PathValidation"
# Look for any security-related comments or documentation
rg -l "SECURITY:|security consideration|secure path"
Length of output: 189
Script:
#!/bin/bash
# Let's investigate the updates package and DLL handling
# Check the updates package implementation
rg -A 5 "GetFile" --type go
# Check for any DLL release or cleanup
rg -A 5 "Release|Unload|Close" --type go
# Look for any existing error handling patterns with DLLs
ast-grep --pattern 'if err := $_.LoadDLL($$$); err != nil { $$$ }'
# Check the integration struct and its cleanup methods
rg -A 10 "type OSIntegration struct" --type go
rg -A 10 "func.*Shutdown|func.*Close|func.*cleanup" --type go
Length of output: 186282
service/integration/module.go
Outdated
| // Start starts the module. | ||
| func (i *OSIntegration) Start() error { | ||
| return i.Initialize() | ||
| } | ||
|
|
||
| // Stop stops the module. | ||
| func (i *OSIntegration) Stop() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
DLL resource leak in Stop() method
The verification reveals that:
Initialize()is properly implemented in both platforms:- Default implementation returns nil
- Windows implementation loads a DLL and initializes ETW functions
Stop()is incomplete as it doesn't properly clean up the loaded DLL on Windows
service/integration/module.go: Add DLL cleanup inStop()method to prevent resource leaks
🔗 Analysis chain
Verify Initialize() implementation and Stop() completeness.
Two potential issues:
Start()callsInitialize()which is not defined in this fileStop()is a no-op, which might not properly clean up resources
Let me gather more information about the Initialize implementation and potential cleanup needs.
Let me check one more thing to understand if there are any DLL or system resources that need cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Initialize() implementation
echo "Searching for Initialize() implementation:"
ast-grep --pattern 'func (*OSIntegration) Initialize() error'
# Search for potential cleanup needs
echo "Checking for resources that might need cleanup:"
rg -l "OSSpecific" --type go
Length of output: 400
Script:
#!/bin/bash
# Check Initialize implementation in other files
echo "Checking Initialize implementation in integration.go:"
rg -A 5 "Initialize\(\)" service/integration/integration.go
echo "Checking Initialize implementation in integration_windows.go:"
rg -A 5 "Initialize\(\)" service/integration/integration_windows.go
# Check for resource allocations or setup that might need cleanup
echo "Checking for resource allocations or setup:"
rg -A 5 "New|Create|Open|Start" service/integration/module.go service/integration/integration.go service/integration/integration_windows.go
Length of output: 1993
Script:
#!/bin/bash
# Check for any DLL or system resource handling
echo "Checking for DLL and system resource handling:"
rg -A 10 "dll|handle|syscall" service/integration/integration_windows.go
# Check if there are any cleanup functions defined
echo "Checking for cleanup functions:"
rg -A 5 "cleanup|close|unload|release" service/integration/integration_windows.go service/integration/integration.go
Length of output: 1089
service/firewall/interception/dnslistener/eventlistener_windows.go
Outdated
Show resolved
Hide resolved
| // Flush flushes the buffer forcing all events to be processed. | ||
| func (dl *DNSListener) Flush() error { | ||
| return dl.listener.flush() | ||
| } |
There was a problem hiding this comment.
Add nil check in Flush method
The Flush method should check if the listener is nil before calling flush() to prevent panic.
func (dl *DNSListener) Flush() error {
+ if dl.listener == nil {
+ return errors.New("listener not initialized")
+ }
return dl.listener.flush()
}📝 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.
| // Flush flushes the buffer forcing all events to be processed. | |
| func (dl *DNSListener) Flush() error { | |
| return dl.listener.flush() | |
| } | |
| // Flush flushes the buffer forcing all events to be processed. | |
| func (dl *DNSListener) Flush() error { | |
| if dl.listener == nil { | |
| return errors.New("listener not initialized") | |
| } | |
| return dl.listener.flush() | |
| } |
| func (etw ETWFunctions) CreateState(callback uintptr) uintptr { | ||
| state, _, _ := etw.createState.Call(callback) | ||
| return state | ||
| } |
There was a problem hiding this comment.
Add error handling in CreateState method
The CreateState method does not check for errors returned by the createState call. This could lead to unhandled errors if the function fails. It's important to verify if the state was created successfully.
Apply this diff to add error handling:
-func (etw ETWFunctions) CreateState(callback uintptr) uintptr {
- state, _, _ := etw.createState.Call(callback)
- return state
+func (etw ETWFunctions) CreateState(callback uintptr) (uintptr, error) {
+ state, _, _ := etw.createState.Call(callback)
+ if state == 0 {
+ return 0, fmt.Errorf("CreateState failed to create state")
+ }
+ return state, nil
}Note: Update the calling code to handle the additional error returned by this method.
📝 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.
| func (etw ETWFunctions) CreateState(callback uintptr) uintptr { | |
| state, _, _ := etw.createState.Call(callback) | |
| return state | |
| } | |
| func (etw ETWFunctions) CreateState(callback uintptr) (uintptr, error) { | |
| state, _, _ := etw.createState.Call(callback) | |
| if state == 0 { | |
| return 0, fmt.Errorf("CreateState failed to create state") | |
| } | |
| return state, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
windows_core_dll/build.ps1 (1)
1-2: Consider adding configuration flexibility and documentationThe script could benefit from additional features for better maintainability and flexibility.
Consider these additional improvements:
+[CmdletBinding()] +param( + [ValidateSet('Release', 'Debug')] + [string]$Configuration = 'Release', + + [ValidateSet('x64', 'Win32')] + [string]$Platform = 'x64' +) + +# Script documentation +<# +.SYNOPSIS + Builds the portmaster-core.dll Windows component. +.DESCRIPTION + Compiles the Windows core DLL that implements ETW session management for DNS events. +.PARAMETER Configuration + Build configuration: 'Release' or 'Debug'. Defaults to 'Release'. +.PARAMETER Platform + Target platform: 'x64' or 'Win32'. Defaults to 'x64'. +#> + +# Build command with flexible configuration +$buildArgs = @( + ".\windows_core_dll.sln", + "/p:Configuration=$Configuration", + "/p:Platform=$Platform" +) + msbuild .\windows_core_dll.sln /p:Configuration=Release -ls .\x64\Release\portmaster-core.dll + +# Output path based on configuration +$outputPath = ".\$Platform\$Configuration\portmaster-core.dll"windows_core_dll/dllmain.cpp (2)
164-174: Correct Function Comment and Spelling Errors inPM_ETWDestroySessionThe comment for
PM_ETWDestroySessionrefers toPM_ETWFlushTraceand contains a spelling error. Accurate comments improve code readability.Apply this diff:
-// PM_ETWFlushTrace Closes the session and frees resourses. +// PM_ETWDestroySession closes the session and frees resources. __declspec(dllexport) uint32_t PM_ETWDestroySession(ETWSessionState* state) { if (state == NULL) { return 1; } uint32_t status = CloseTrace(state->sessionHandle); // Free memory. free(state->SessionProperties); free(state); return status; }
1-197: Unit Tests Needed for ETW Session FunctionsThe new ETW session management functions lack unit tests. Adding unit tests will help ensure their reliability and make future maintenance easier.
Would you like assistance in creating unit tests for these functions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
windows_core_dll/build.ps1(1 hunks)windows_core_dll/dllmain.cpp(1 hunks)windows_core_dll/framework.h(1 hunks)windows_core_dll/pch.cpp(1 hunks)windows_core_dll/pch.h(1 hunks)windows_core_dll/windows_core_dll.sln(1 hunks)windows_core_dll/windows_core_dll.vcxproj(1 hunks)windows_core_dll/windows_core_dll.vcxproj.filters(1 hunks)windows_core_dll/windows_core_dll.vcxproj.user(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- windows_core_dll/framework.h
- windows_core_dll/pch.cpp
- windows_core_dll/pch.h
- windows_core_dll/windows_core_dll.sln
- windows_core_dll/windows_core_dll.vcxproj
- windows_core_dll/windows_core_dll.vcxproj.filters
- windows_core_dll/windows_core_dll.vcxproj.user
🔇 Additional comments (2)
windows_core_dll/dllmain.cpp (2)
25-33: Consider 32-bit Compatibility for Callback Function Pointer
The GoEventRecordCallback typedef uses uint64_t as the return type, which might cause issues on 32-bit systems. Ensure that the code is compatible with both 32-bit and 64-bit architectures.
158-161: 🛠️ Refactor suggestion
Incorrect Function Name in Comment
The comment for PM_ETWStopTrace incorrectly refers to PM_ETWFlushTrace. This could cause confusion when maintaining the code.
Apply this diff to correct the comment:
-// PM_ETWFlushTrace stops the listener.
+// PM_ETWStopTrace stops the listener.
__declspec(dllexport) uint32_t PM_ETWStopTrace(ETWSessionState* state) {
return ControlTrace(state->SessionTraceHandle, LOGSESSION_NAME, state->SessionProperties,
EVENT_TRACE_CONTROL_STOP);
}Likely invalid or redundant comment.
| __declspec(dllexport) uint32_t PM_ETWInitializeSession(ETWSessionState* state) { | ||
| return StartTrace(&state->SessionTraceHandle, LOGSESSION_NAME, | ||
| state->SessionProperties); | ||
| } |
There was a problem hiding this comment.
Null Pointer Check Needed in PM_ETWInitializeSession
The function PM_ETWInitializeSession does not verify if the state pointer or state->SessionProperties is NULL before using them. Dereferencing a NULL pointer will lead to a crash.
Apply this diff to add null checks:
__declspec(dllexport) uint32_t PM_ETWInitializeSession(ETWSessionState* state) {
+ if (state == NULL || state->SessionProperties == NULL) {
+ return ERROR_INVALID_PARAMETER;
+ }
return StartTrace(&state->SessionTraceHandle, LOGSESSION_NAME,
state->SessionProperties);
}📝 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.
| __declspec(dllexport) uint32_t PM_ETWInitializeSession(ETWSessionState* state) { | |
| return StartTrace(&state->SessionTraceHandle, LOGSESSION_NAME, | |
| state->SessionProperties); | |
| } | |
| __declspec(dllexport) uint32_t PM_ETWInitializeSession(ETWSessionState* state) { | |
| if (state == NULL || state->SessionProperties == NULL) { | |
| return ERROR_INVALID_PARAMETER; | |
| } | |
| return StartTrace(&state->SessionTraceHandle, LOGSESSION_NAME, | |
| state->SessionProperties); | |
| } |
| __declspec(dllexport) ETWSessionState* PM_ETWCreateState(GoEventRecordCallback callback) { | ||
| // Create trace session properties. | ||
| ULONG BufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | ||
| EVENT_TRACE_PROPERTIES* SessionProperties = | ||
| (EVENT_TRACE_PROPERTIES*)calloc(1, BufferSize); | ||
| SessionProperties->Wnode.BufferSize = BufferSize; | ||
| SessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | ||
| SessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | ||
| SessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | ||
| SessionProperties->LogFileMode = EVENT_TRACE_REAL_TIME_MODE; | ||
| SessionProperties->MaximumFileSize = 1; // MB | ||
| SessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | ||
|
|
||
| // Create state | ||
| ETWSessionState* state = (ETWSessionState*)calloc(1, sizeof(ETWSessionState)); | ||
| state->SessionProperties = SessionProperties; | ||
| state->callback = callback; | ||
| return state; | ||
| } |
There was a problem hiding this comment.
Add Error Handling for Memory Allocation in PM_ETWCreateState
The function PM_ETWCreateState allocates memory using calloc for SessionProperties and state without checking if the allocation was successful. If memory allocation fails, dereferencing a NULL pointer will cause a crash.
Apply this diff to add error checks:
ULONG BufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME);
EVENT_TRACE_PROPERTIES* SessionProperties =
(EVENT_TRACE_PROPERTIES*)calloc(1, BufferSize);
+if (SessionProperties == NULL) {
+ // Handle memory allocation failure
+ return NULL;
+}
SessionProperties->Wnode.BufferSize = BufferSize;
// ... [rest of the code remains unchanged]
// Create state
ETWSessionState* state = (ETWSessionState*)calloc(1, sizeof(ETWSessionState));
+if (state == NULL) {
+ // Clean up previously allocated memory
+ free(SessionProperties);
+ return NULL;
+}
state->SessionProperties = SessionProperties;
state->callback = callback;
return state;📝 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.
| __declspec(dllexport) ETWSessionState* PM_ETWCreateState(GoEventRecordCallback callback) { | |
| // Create trace session properties. | |
| ULONG BufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | |
| EVENT_TRACE_PROPERTIES* SessionProperties = | |
| (EVENT_TRACE_PROPERTIES*)calloc(1, BufferSize); | |
| SessionProperties->Wnode.BufferSize = BufferSize; | |
| SessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | |
| SessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | |
| SessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | |
| SessionProperties->LogFileMode = EVENT_TRACE_REAL_TIME_MODE; | |
| SessionProperties->MaximumFileSize = 1; // MB | |
| SessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | |
| // Create state | |
| ETWSessionState* state = (ETWSessionState*)calloc(1, sizeof(ETWSessionState)); | |
| state->SessionProperties = SessionProperties; | |
| state->callback = callback; | |
| return state; | |
| } | |
| __declspec(dllexport) ETWSessionState* PM_ETWCreateState(GoEventRecordCallback callback) { | |
| // Create trace session properties. | |
| ULONG BufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | |
| EVENT_TRACE_PROPERTIES* SessionProperties = | |
| (EVENT_TRACE_PROPERTIES*)calloc(1, BufferSize); | |
| if (SessionProperties == NULL) { | |
| // Handle memory allocation failure | |
| return NULL; | |
| } | |
| SessionProperties->Wnode.BufferSize = BufferSize; | |
| SessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | |
| SessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | |
| SessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | |
| SessionProperties->LogFileMode = EVENT_TRACE_REAL_TIME_MODE; | |
| SessionProperties->MaximumFileSize = 1; // MB | |
| SessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | |
| // Create state | |
| ETWSessionState* state = (ETWSessionState*)calloc(1, sizeof(ETWSessionState)); | |
| if (state == NULL) { | |
| // Clean up previously allocated memory | |
| free(SessionProperties); | |
| return NULL; | |
| } | |
| state->SessionProperties = SessionProperties; | |
| state->callback = callback; | |
| return state; | |
| } |
| if conn.Entity.Port == 53 && conn.Entity.IPScope.IsLocalhost() { | ||
| return endpoints.NoMatch, "", nil | ||
| } |
There was a problem hiding this comment.
I think we can allow any normal DNS, as we can see the packets. We only need to prevent encrypted DNS connections from apps.
This is getting complicated. Maybe we can simplify and regroup the logic here? Let's talk about options.
| } | ||
|
|
||
| // Check if the reply indicates the end of the stream | ||
| if flags&varlink.Continues == 0 { |
There was a problem hiding this comment.
If the flag is set, shouldn't the value be > 0, or is this good old 0 == true?
There was a problem hiding this comment.
I don't like the way gofmt formats this.
This checks if the 3-th bit is 1 with an and mask.
varlink.Contunues == 0b00000100
| Name string `json:"name"` | ||
| } | ||
|
|
||
| type ResourceRecord struct { |
There was a problem hiding this comment.
Do we have a source where this came from?
Would be great to link in case something breaks and we need to check if this is still correct.
There was a problem hiding this comment.
The source is:
sudo varlinkctl introspect /run/systemd/resolve/io.systemd.Resolve.Monitor io.systemd.Resolve.Monitor
In varlink the service is required to provide a description of its functions.
I will add a comment
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (32)
service/integration/integration.go (3)
6-6: Add documentation for the OSSpecific type.Consider adding a doc comment explaining the purpose of this type and its relationship with the Windows implementation.
+// OSSpecific provides a platform-specific implementation of OS integration features. +// This is a no-op implementation for non-Windows platforms. type OSSpecific struct{}
8-11: Fix typo and enhance method documentation.The documentation contains a typo and could be more descriptive.
-// Initialize is empty on any OS different then Windows. +// Initialize performs OS-specific initialization. +// This is a no-op implementation for non-Windows platforms. func (i *OSIntegration) Initialize() error {
13-16: Fix typos in CleanUp method documentation.The documentation contains spelling errors.
-// CleanUp releases any resourses allocated during initializaion. +// CleanUp releases any resources allocated during initialization. +// This is a no-op implementation for non-Windows platforms. func (i *OSIntegration) CleanUp() error {service/integration/module.go (1)
45-53: Improve error handling in lifecycle methodsThe Start() and Stop() methods should include proper state management and logging to aid in debugging and ensure clean shutdown.
Consider this enhancement:
func (i *OSIntegration) Start() error { + i.m.StartWorker("os-integration") + defer func() { + if err := recover(); err != nil { + i.m.Error("panic during start: %v", err) + } + }() return i.Initialize() } func (i *OSIntegration) Stop() error { + defer i.m.StopWorker() + defer func() { + if err := recover(); err != nil { + i.m.Error("panic during shutdown: %v", err) + } + }() return i.CleanUp() }service/integration/integration_windows.go (3)
13-16: Add godoc comments for the struct and its fields.The struct is well-designed, but could benefit from documentation explaining its purpose and field usage.
+// OSSpecific holds Windows-specific integration components type OSSpecific struct { + // dll holds the loaded portmaster-core.dll instance dll *windows.DLL + // etwFunctions contains the ETW-related function pointers etwFunctions ETWFunctions }
41-47: Fix typo and improve error handling in CleanUp method
- The comment has typos: "resourses" should be "resources" and "initializaion" should be "initialization"
- Consider wrapping the error with context
-// CleanUp releases any resourses allocated during initializaion. +// CleanUp releases any resources allocated during initialization. func (i *OSIntegration) CleanUp() error { if i.os.dll != nil { - return i.os.dll.Release() + if err := i.os.dll.Release(); err != nil { + return fmt.Errorf("failed to release DLL: %w", err) + } } return nil }
49-52: Fix comment typo and improve documentationThe comment has a typo and could be more descriptive.
-// GetETWInterface return struct containing all the ETW related functions. +// GetETWInterface returns a struct containing all the ETW (Event Tracing for Windows) related functions. func (i *OSIntegration) GetETWInterface() ETWFunctions { return i.os.etwFunctions }service/firewall/interception/dnsmonitor/varlinktypes.go (4)
1-8: Consider enhancing the documentation with protocol version and link.While the source comment is helpful, it would be beneficial to include:
- The protocol version or release date
- A link to the official systemd-resolver documentation
- Examples of typical DNS event structures
9-13: Add field documentation and consider using typed constants.
- Add documentation for each field explaining their purpose and valid values.
- Consider using typed constants for Class and Type fields to prevent invalid values:
+// Common DNS classes +type DNSClass int +const ( + ClassIN DNSClass = 1 // Internet + ClassCS DNSClass = 2 // CSNET + ClassCH DNSClass = 3 // CHAOS + ClassHS DNSClass = 4 // Hesiod +) + +// Common DNS types +type DNSType int +const ( + TypeA DNSType = 1 // IPv4 address + TypeNS DNSType = 2 // Nameserver + TypeCNAME DNSType = 5 // Canonical name + TypeSOA DNSType = 6 // Start of authority + TypePTR DNSType = 12 // Domain name pointer + TypeMX DNSType = 15 // Mail exchange + TypeTXT DNSType = 16 // Text strings + TypeAAAA DNSType = 28 // IPv6 address +) type ResourceKey struct { - Class int `json:"class"` - Type int `json:"type"` + Class DNSClass `json:"class"` + Type DNSType `json:"type"` Name string `json:"name"` }
69-73: Add struct and field documentation.The Answer struct would benefit from documentation explaining:
- The purpose of the struct
- Why Raw is a required field while others are optional
- The relationship between RR and Raw fields
- The significance of IfIndex in DNS responses
Example:
// Answer represents a DNS response containing either a parsed resource record (RR), // the raw response string, or both. IfIndex optionally specifies the network interface // through which the response was received. type Answer struct { RR *ResourceRecord `json:"rr,omitempty"` // Parsed resource record, if available Raw string `json:"raw"` // Raw DNS response string IfIndex *int `json:"ifindex,omitempty"` // Network interface index, if applicable }
75-83: Add documentation and clarify field relationships.
Add comprehensive documentation explaining:
- The purpose of each field
- The relationship between Question and CollectedQuestions
- Valid values for Ready, State, Rcode, and Errno fields
Consider adding constants for common values:
// Query states const ( StateComplete = "complete" StatePending = "pending" StateFailed = "failed" ) // DNS response codes (RCODEs) const ( RcodeNoError = 0 RcodeFormatError = 1 RcodeServerFailure = 2 RcodeNameError = 3 RcodeNotImplemented = 4 RcodeRefused = 5 )service/compat/module.go (3)
102-106: LGTM! Consider standardizing error handling.The early check for disabled resolver is well-placed and prevents unnecessary work. However, for consistency with other skip conditions in this function, consider using the
errSelfcheckSkippederror instead of returning nil.if res.IsDisabled() { log.Debugf("compat: skipping self-check: resolver is disabled") - return nil + return errSelfcheckSkipped }
127-128: Add debug logging for consistency.While the logic is correct, this case lacks debug logging unlike other similar conditions (e.g., network change). This inconsistency could make debugging more difficult.
case res.IsDisabled(): + tracer.Debugf("compat: ignoring self-check failure: resolver is disabled") // Portmaster resolver is disabled, ignore this issue.
Line range hint
41-52: Consider reducing bidirectional package dependencies.The init() function shows that the resolver package depends on compat package functions, and now compat also depends on resolver. This bidirectional dependency could make the system harder to maintain and understand.
Consider introducing an interface layer or moving shared functionality to a separate package to reduce coupling.
service/instance.go (2)
158-161: Maintain consistency in error messages.While the initialization code follows the established pattern, there's a minor inconsistency in the error message format:
- Integration module uses: "create integration module"
- DNS monitor uses: "create dns-listener module"
The module name in the error message should match the struct field name for consistency.
- return instance, fmt.Errorf("create dns-listener module: %w", err) + return instance, fmt.Errorf("create dnsmonitor module: %w", err)Also applies to: 198-201
485-488: Maintain consistency in method comments.For consistency with the struct field name, update the comment:
-// DNSMonitor returns the dns-listener module. +// DNSMonitor returns the dnsmonitor module.service/resolver/resolvers.go (1)
401-408: Consider refactoringloadResolversfor better maintainability.The
loadResolversfunction is quite long and handles multiple responsibilities. Consider breaking it down into smaller, focused functions:
initializeResolverStateloadResolverConfigurationinitializeResolverslogResolverStatusThis would improve:
- Code maintainability
- Testing capabilities
- Error handling granularity
Example refactor for the state management:
+func initializeResolverState(config []string) { + if len(config) == 0 { + module.isDisabled.Store(true) + } else { + module.isDisabled.Store(false) + } +} + func loadResolvers() { defer func() { if !allConfiguredResolversAreFailing() { resetFailingResolversNotification() } }() resolversLock.Lock() defer resolversLock.Unlock() module.states.Remove(missingResolversErrorID) newResolverConfig := configuredNameServers() if len(currentResolverConfig) > 0 && !utils.StringSliceEqual(currentResolverConfig, newResolverConfig) { module.mgr.Go("clear dns cache", func(ctx *mgr.WorkerCtx) error { log.Info("resolver: clearing dns cache due to changed resolver config") _, err := clearNameCache(ctx.Ctx()) return err }) } - if len(newResolverConfig) == 0 { - module.isDisabled.Store(true) - } else { - module.isDisabled.Store(false) - } + initializeResolverState(newResolverConfig)Also applies to: 441-441
service/firewall/interception/dnsmonitor/eventlistener_windows.go (5)
23-23: Correct typographical error in comment.On line 23, the comment contains a typo: "Intialize" should be "Initialize."
Apply this diff to fix the typo:
- // Intialize new dns event session. + // Initialize new DNS event session.
29-29: Correct typographical error in comment.On line 29, the comment contains a typo: "lisening" should be "listening."
Apply this diff to fix the typo:
- // Start lisening for events. + // Start listening for events.
48-48: Correct typographical errors in comment.On line 48, the comment contains typos: "Destory" should be "Destroy," and "failes" should be "fails."
Apply this diff to correct the typos:
- // Stop and destroy trace. Destory should be called even if stop failes for some reason. + // Stop and destroy trace. Destroy should be called even if stop fails for some reason.
57-57: Fix typographical error in error message.On line 57, the error message contains a typo: "DestorySession" should be "DestroySession."
Apply this diff to fix the typo:
- return fmt.Errorf("DestorySession failed: %w", err2) + return fmt.Errorf("DestroySession failed: %w", err2)
49-59: Consider combining errors fromStopTraceandDestroySession.Currently, if both
StopTraceandDestroySessionfail, only one error is returned, and the other is discarded. To provide comprehensive error information, consider combining both errors.Here's a possible way to handle both errors:
err := l.etw.StopTrace() err2 := l.etw.DestroySession() if err != nil || err2 != nil { var combinedError string if err != nil { combinedError += fmt.Sprintf("StopTrace failed: %v. ", err) } if err2 != nil { combinedError += fmt.Sprintf("DestroySession failed: %v.", err2) } return fmt.Errorf(combinedError) } return nilservice/firewall/interception/dnsmonitor/eventlistener_linux.go (5)
21-21: Consider making Varlink service address configurableCurrently, the Varlink connection is established using a hardcoded address
"unix:/run/systemd/resolve/io.systemd.Resolve.Monitor". To improve flexibility and adaptability across different environments, consider making this address configurable or deriving it from a constant.
32-38: Include method name in error messages for clarityWhen handling errors from the Varlink call, including the method name
"io.systemd.Resolve.Monitor.SubscribeQueryResults"in the error messages can provide more context and assist in debugging.
58-60: Log ignored DNS errors for monitoringCurrently, DNS errors are ignored when
queryResult.Rcode != nil. Consider logging these errors to monitor DNS issues, which can be valuable for troubleshooting and performance analysis.
82-82: Correct the comment grammarThe comment should read "Allocate data structures for the parsed result." instead of "Allocated data struct for the parsed result."
93-93: Fix typo in comment: "trough" to "through"There's a typo in the comment. Replace "Go trough each answer entry." with "Go through each answer entry."
service/network/connection.go (1)
552-561: Log a message if DNS retries failIf all retries to obtain DNS information fail, currently no message is logged. Adding a log message can aid in debugging and monitoring potential issues with DNS resolution.
Apply this diff to add a log message after all retries have been exhausted:
} time.Sleep(5 * time.Millisecond) } + if err != nil { + log.Tracer(pkt.Ctx()).Warningf("network: failed to retrieve domain after %d retries", i) + } }service/firewall/interception/dnsmonitor/etwlink_windows.go (4)
25-54: Correct spelling mistakes in comments and error messagesThere are typographical errors in the
NewSessionfunction:
- Line 25~: "initilizes" should be "initializes", and "DestorySession" should be "DestroySession".
- Line 50~: "initialzie" should be "initialize".
Correcting these typos will enhance code readability and professionalism.
79-79: Fix typographical error in method commentIn the comment for
StopTrace(line 79~), "stopes" should be "stops".
43-45: Avoid relying on finalizers for critical resource cleanupUsing
runtime.SetFinalizerto callDestroySessionmay not guarantee timely release of resources, as finalizers run unpredictably during garbage collection. This can lead to resource leaks or delayed cleanup.It's recommended to ensure that
DestroySessionis explicitly called when the session is no longer needed. Consider removing the finalizer or clarifying in the documentation the importance of callingDestroySessionpromptly.
25-99: Follow GoDoc conventions for method commentsFor improved documentation and consistency, method comments should start with the method name. For example:
- Change (line 25~):
To:// NewSession creates new ETW event listener and initilizes it.// NewSession creates a new ETW event listener and initializes it.Apply this pattern to all method comments to adhere to GoDoc standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
service/compat/module.go(3 hunks)service/firewall/bypassing.go(1 hunks)service/firewall/interception/dnsmonitor/etwlink_windows.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_linux.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_windows.go(1 hunks)service/firewall/interception/dnsmonitor/module.go(1 hunks)service/firewall/interception/dnsmonitor/varlinktypes.go(1 hunks)service/firewall/packet_handler.go(1 hunks)service/instance.go(9 hunks)service/integration/etw_windows.go(1 hunks)service/integration/integration.go(1 hunks)service/integration/integration_windows.go(1 hunks)service/integration/module.go(1 hunks)service/network/connection.go(4 hunks)service/network/module.go(2 hunks)service/resolver/main.go(2 hunks)service/resolver/resolvers.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- service/firewall/interception/dnsmonitor/eventlistener.go
🚧 Files skipped from review as they are similar to previous changes (4)
- service/firewall/bypassing.go
- service/firewall/packet_handler.go
- service/integration/etw_windows.go
- service/resolver/main.go
🧰 Additional context used
🪛 golangci-lint
service/instance.go
395-395: intergration is a misspelling of integration
(misspell)
🔇 Additional comments (14)
service/integration/integration.go (1)
1-2: LGTM! Build constraints follow Go's compatibility guidelines.
The dual build constraints properly handle both new and legacy Go versions.
service/integration/module.go (2)
11-20: Previous review comment about nil pointer is still valid
The os field initialization issue mentioned in the previous review remains unaddressed. This could lead to runtime panics if the field is accessed before being properly set.
46-53: Verify platform-specific implementations
The Start() and Stop() methods delegate to Initialize() and CleanUp(), which are platform-specific. Let's verify their implementations.
✅ Verification successful
Based on the search results, I can see that the code has proper cleanup handling for resources:
- The
Initialize()andCleanUp()methods are implemented in both Windows and non-Windows platforms:
- Windows (
integration_windows.go): Properly releases DLL resources inCleanUp() - Non-Windows (
integration.go): Has empty implementations since no resources need cleanup
-
The
Start()andStop()methods inmodule.gocorrectly delegate toInitialize()andCleanUp()respectively. -
The Windows implementation properly handles DLL loading and unloading:
- Loads DLL in
Initialize() - Releases DLL in
CleanUp()when it exists
- The error handling is appropriate, with errors being propagated up the call chain.
The implementation follows proper resource management practices, with resources being initialized and cleaned up in a symmetric way. The Windows-specific code properly manages native resources through the DLL lifecycle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check platform-specific implementations of Initialize and CleanUp
echo "Checking Initialize() implementations:"
rg -A 5 "Initialize\(\)" --type go
echo "Checking CleanUp() implementations:"
rg -A 5 "CleanUp\(\)" --type go
echo "Checking for potential resource handling:"
rg -A 5 "(Open|Close|Load|Unload)" --type go
Length of output: 272295
service/integration/integration_windows.go (2)
1-11: LGTM! Build constraints and imports are properly configured.
The file correctly uses both old and new style build constraints for Windows, and imports the necessary packages.
19-39:
Add cleanup handling for initialization failures
The initialization process needs proper cleanup in case of failures after the DLL is loaded. This prevents resource leaks.
Apply this fix as suggested in the previous review:
func (i *OSIntegration) Initialize() error {
// Find path to the dll.
file, err := updates.GetFile("portmaster-core.dll")
if err != nil {
return err
}
// Load the DLL.
i.os.dll, err = windows.LoadDLL(file.Path())
if err != nil {
return fmt.Errorf("failed to load dll: %q", err)
}
+ // Ensure cleanup on failure
+ var success bool
+ defer func() {
+ if !success {
+ i.os.dll.Release()
+ i.os.dll = nil
+ }
+ }()
// Enumerate all needed dll functions.
i.os.etwFunctions, err = initializeETW(i.os.dll)
if err != nil {
return err
}
+ success = true
return nil
}service/firewall/interception/dnsmonitor/varlinktypes.go (1)
1-83: Verify protocol compatibility and completeness.
Let's verify that this implementation covers all required DNS record types and matches the systemd-resolver protocol specification.
✅ Verification successful
Let me analyze the DNS record types and protocol compatibility based on the search results.
The code in varlinktypes.go defines structs for systemd-resolver varlink DNS event protocol, but we need to verify if it covers all required DNS record types and matches the protocol specification. Let me search for more specific information about DNS record type handling.
Based on the search results, I can now provide a final verification response:
Implementation matches protocol specification but has limited record type support
The implementation correctly follows the systemd-resolver varlink DNS event protocol as documented in the source comment (sudo varlinkctl introspect /run/systemd/resolve/io.systemd.Resolve.Monitor). However:
-
The code currently only actively uses a subset of DNS record types:
- A and AAAA records for IPv4/IPv6 addresses
- CNAME records for aliases
- PTR records for reverse lookups
- SOA records for zone information
- SVCB and HTTPS records for service binding
- OPT and NULL records for protocol extensions
-
The
ResourceRecordstruct includes commented fields for many other record types (MX, NS, TXT, etc.) but they are marked as "not used"
This implementation is sufficient for basic DNS resolution but may need to uncomment and implement handling for additional record types if more comprehensive DNS record support is required in the future.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DNS record types coverage and protocol compatibility
# Check if we have all common DNS record types
echo "Checking for common DNS record types in the code..."
rg "Type[A-Z]+" -A 1
# Look for systemd-resolver protocol documentation or tests
echo "Looking for systemd-resolver protocol documentation..."
fd -g "*resolve*.varlink" -x cat {} \;
fd -g "*dns*.varlink" -x cat {} \;
# Search for related test files
echo "Looking for related test files..."
fd -g "*dns*test*.go" -x cat {} \;
fd -g "*resolve*test*.go" -x cat {} \;
Length of output: 92507
Script:
#!/bin/bash
# Search for DNS record type handling in the codebase
# Look for DNS record type constants and usage
echo "Checking DNS record type handling..."
rg "dns\.Type[A-Z]+" -A 1
# Look for systemd-resolver protocol documentation
echo "Looking for systemd-resolver protocol documentation..."
rg -i "systemd.*resolver.*protocol" -A 3
rg -i "varlink.*protocol" -A 3
# Look for any additional DNS record type handling
echo "Looking for additional DNS record type handling..."
rg "ResourceRecord" -A 5
Length of output: 5723
service/network/module.go (2)
12-12: LGTM: Import statements are correctly added
The new imports for dnsmonitor and resolver packages are properly placed and align with the interface enhancements.
Also applies to: 17-17
193-194: Verify implementations of new interface methods
The interface additions look good, but all implementers of the instance interface must be updated to include these methods.
Let's verify the implementations:
✅ Verification successful
Interface methods are properly implemented
The verification shows that all new interface methods are correctly implemented in the main Instance struct in service/instance.go:
Profile()implementation existsResolver()implementation existsDNSMonitor()implementation exists
No other implementers of the interface were found in the codebase, indicating that Instance is the only implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementers of the instance interface and verify method implementations
# Find all types that implement Profile() method (existing interface method)
echo "=== Searching for instance interface implementers ==="
ast-grep --pattern 'func ($receiver *$name) Profile() *profile.ProfileModule {
$$$
}'
# Verify new method implementations
echo -e "\n=== Checking Resolver() implementation ==="
ast-grep --pattern 'func ($receiver *$name) Resolver() *resolver.ResolverModule {
$$$
}'
echo -e "\n=== Checking DNSMonitor() implementation ==="
ast-grep --pattern 'func ($receiver *$name) DNSMonitor() *dnsmonitor.DNSMonitor {
$$$
}'
# Check test implementations
echo -e "\n=== Checking test implementations ==="
fd -e go -x grep -l "type.*struct.*testing.T"
Length of output: 737
Script:
#!/bin/bash
# Let's try a different approach to find implementers and their methods
# First, find all Go files that might contain implementations
echo "=== Finding relevant Go files ==="
rg -l "type.*?struct" --type go
# Then search for Profile method to identify implementers
echo -e "\n=== Finding Profile() implementations ==="
rg "func \(.*?\) Profile\(\) \*profile\.ProfileModule" -A 1
# Search for new method implementations
echo -e "\n=== Finding Resolver() implementations ==="
rg "func \(.*?\) Resolver\(\) \*resolver\.ResolverModule" -A 1
echo -e "\n=== Finding DNSMonitor() implementations ==="
rg "func \(.*?\) DNSMonitor\(\) \*dnsmonitor\.DNSMonitor" -A 1
# Look for mock implementations in test files
echo -e "\n=== Finding mock implementations in tests ==="
rg "type.*?Mock.*?struct" --type go
Length of output: 12673
service/instance.go (3)
22-23: LGTM: Import statements are properly organized.
The new imports are correctly placed and follow the existing grouping pattern.
70-70: LGTM: Fields are properly organized within the Instance struct.
The new fields are correctly placed near related modules, maintaining the logical grouping of the struct fields.
Also applies to: 80-80
290-290: LGTM: Modules are properly registered in the service group.
The new modules are correctly placed near related modules, maintaining the logical grouping in the service initialization.
Also applies to: 304-304
service/resolver/resolvers.go (2)
441-441: LGTM! Fixed typo in comment.
The comment text has been corrected from "assing" to "assign".
401-408: Verify the impact of resolver disabled state on dependent modules.
The new code correctly manages the resolver's disabled state, but we should verify how other modules handle this state change.
✅ Verification successful
Based on the search results, I can now generate a final response:
The resolver's disabled state management is safely implemented and properly consumed
The disabled state is:
- Correctly managed using atomic operations in
service/resolver/resolvers.go - Only checked via the exported
IsDisabled()method inservice/resolver/main.go - Used by the UI layer to reflect system state in
desktop/angular/src/app/shared/config
The implementation is thread-safe and follows a clear pattern where state changes are properly propagated to dependent modules through the UI layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find modules that check the resolver's disabled state
# to ensure they handle the state change appropriately.
# Search for references to the disabled state
rg -A 5 "isDisabled"
# Search for direct resolver state checks
ast-grep --pattern 'if $resolver.$state {
$$$
}'
Length of output: 5188
service/firewall/interception/dnsmonitor/module.go (1)
77-85:
Correct loop syntax in saveDomain function
The loop for range 50 is not correctly iterating 50 times. To loop a fixed number of times, use a counter-controlled loop.
Apply this diff to fix the loop:
- for range 50 {
+ for i := 0; i < 50; i++ {
nextDomain, isCNAME := cnames[domain]
if !isCNAME || nextDomain == domain {
break
}
record.CNAMEs = append(record.CNAMEs, nextDomain)
domain = nextDomain
}Likely invalid or redundant comment.
| @@ -181,4 +189,5 @@ func New(instance instance) (*Compat, error) { | |||
|
|
|||
| type instance interface { | |||
| NetEnv() *netenv.NetEnv | |||
| Resolver() *resolver.ResolverModule | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing Resolver() implementation in spn/instance.go
The verification reveals that spn/instance.go implements NetEnv() but lacks the newly added Resolver() method. This implementation needs to be updated to maintain interface compatibility.
spn/instance.go: Add the missingResolver()method to match the interface requirements- Other implementations in
service/instance.goand test files are correctly updated
🔗 Analysis chain
Verify interface implementation across the codebase.
The addition of Resolver() method to the interface looks good, but we should verify that all types implementing this interface have been updated.
The previous attempt might have failed due to complex AST pattern. Let's try using ripgrep to:
- Find the interface definition to confirm the methods we need to check
- Search for implementations of key methods (NetEnv and Resolver) to identify structs that implement them
The previous output shows we have potentially found an issue - the spn/instance.go implementation has NetEnv() but might be missing Resolver(). Let's verify the complete interface definition and check the full implementation in spn/instance.go to confirm this.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for types that implement the instance interface
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) NetEnv() *netenv.NetEnv {
$$$
}'
# This will help identify structs that implement NetEnv() but might be missing Resolver()
Length of output: 95
Script:
#!/bin/bash
# Let's try a different approach to find interface implementations
# First, let's find the interface definition
echo "=== Interface Definition ==="
rg -A 5 "type Instance interface"
# Then search for struct types that might implement it
echo -e "\n=== Potential Implementations ==="
rg "func \(.*?\) (NetEnv|Resolver)\(\)" --type go -A 1
Length of output: 695
Script:
#!/bin/bash
# Let's find the complete interface definition first
echo "=== Complete Interface Definition ==="
rg -B 2 -A 10 "type Instance interface"
# Let's also check the implementation in spn/instance.go since it's missing Resolver()
echo -e "\n=== SPN Instance Implementation ==="
rg -B 2 -A 15 "type Instance struct" spn/instance.go
Length of output: 649
| return dl.listener.flush() | ||
| } | ||
|
|
There was a problem hiding this comment.
Check for nil listener before flushing in Flush() method
The Flush() method should check if dl.listener is not nil before calling flush(), to prevent a potential nil pointer dereference.
Apply this diff to add a nil check:
func (dl *DNSMonitor) Flush() error {
+ if dl.listener != nil {
return dl.listener.flush()
+ }
+ return nil
}📝 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.
| return dl.listener.flush() | |
| } | |
| func (dl *DNSMonitor) Flush() error { | |
| if dl.listener != nil { | |
| return dl.listener.flush() | |
| } | |
| return nil | |
| } |
| func (l *ETWSession) StartTrace() error { | ||
| return l.i.StartTrace(l.state) | ||
| } |
There was a problem hiding this comment.
Prevent method calls after session is destroyed
The methods StartTrace (lines 57-59~) and StopTrace (lines 81-82~) do not check if the session has been destroyed before proceeding. Calling these methods after DestroySession could result in undefined behavior or errors due to the invalid l.state.
Add checks to ensure the session is still running before executing these methods.
Apply the following changes to add safety checks:
func (l *ETWSession) StartTrace() error {
+ if l.shutdownGuard.Load() {
+ return fmt.Errorf("cannot start trace; session has been destroyed")
+ }
return l.i.StartTrace(l.state)
}
func (l *ETWSession) StopTrace() error {
+ if l.shutdownGuard.Load() {
+ return fmt.Errorf("cannot stop trace; session has been destroyed")
+ }
return l.i.StopTrace(l.state)
}Also applies to: 81-82
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
service/integration/module.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint
service/integration/module.go
13-13: field os is unused
(unused)
🔇 Additional comments (3)
service/integration/module.go (3)
1-6: Clean package structure and imports
36-44: Improve lifecycle methods with proper error handling and documentation
The Start() and Stop() methods have several concerns:
- They depend on undefined methods (Initialize and CleanUp)
- No error handling or logging
- Potential resource leaks in Stop() as mentioned in previous reviews
Please:
- Document where Initialize() and CleanUp() are implemented
- Add error handling and logging
- Ensure proper resource cleanup in Stop()
46-48: Clean interface definition
The interface follows good design practices with a single, focused responsibility.
Copilot
AI
left a comment
Copilot
AI
left a comment
There was a problem hiding this comment.
Copilot reviewed 16 out of 31 changed files in this pull request and generated 3 suggestions.
Files not reviewed (15)
- go.mod: Language not supported
- service/network/module.go: Evaluated as low risk
- service/network/connection.go: Evaluated as low risk
- service/firewall/packet_handler.go: Evaluated as low risk
- service/resolver/main.go: Evaluated as low risk
- service/firewall/module.go: Evaluated as low risk
- service/firewall/bypassing.go: Evaluated as low risk
- service/instance.go: Evaluated as low risk
- service/compat/module.go: Evaluated as low risk
- service/resolver/resolvers.go: Evaluated as low risk
- service/firewall/interception/dnsmonitor/varlinktypes.go: Evaluated as low risk
- service/firewall/interception/dnsmonitor/eventlistener.go: Evaluated as low risk
- service/integration/integration.go: Evaluated as low risk
- service/integration/etw_windows.go: Evaluated as low risk
- service/integration/integration_windows.go: Evaluated as low risk
Comments skipped due to low confidence (2)
service/firewall/interception/dnsmonitor/eventlistener_windows.go:43
- [nitpick] The error message 'listener is nil' is not very descriptive. Consider changing it to 'Listener instance is nil'.
return fmt.Errorf("listener is nil")
service/firewall/interception/dnsmonitor/eventlistener_windows.go:46
- [nitpick] The error message 'invalid etw session' is not very descriptive. Consider changing it to 'ETW session instance is invalid'.
return fmt.Errorf("invalid etw session")
service/firewall/interception/dnsmonitor/eventlistener_linux.go
Outdated
Show resolved
Hide resolved
service/firewall/interception/dnsmonitor/eventlistener_windows.go
Outdated
Show resolved
Hide resolved
service/firewall/interception/dnsmonitor/eventlistener_windows.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (20)
service/firewall/interception/windowskext2/packet.go (1)
23-23: Use named constants for payload layer values to enhance readabilityConsider defining named constants for the payload layer values
3and4to improve code readability and maintainability.Apply this diff to define constants and use them:
+const ( + LayerIPv4 uint8 = 4 + LayerIPv6 uint8 = 6 +)Update the
payloadLayerfield and the switch statement:type Packet struct { packet.Base verdictRequest uint64 payload []byte - payloadLayer uint8 + payloadLayer uint8 // Use LayerIPv4 or LayerIPv6 verdictSet *abool.AtomicBool payloadLoaded bool lock sync.Mutex } switch pkt.payloadLayer { - case 3: - err = packet.ParseLayer3(pkt.payload, &pkt.Base) - case 4: + case LayerIPv4: err = packet.ParseLayer4(pkt.payload, &pkt.Base) + case LayerIPv6: + err = packet.ParseLayer6(pkt.payload, &pkt.Base) default: err = fmt.Errorf("unsupported payload layer: %d", pkt.payloadLayer) }Also applies to: 58-64
service/firewall/interception/dnsmonitor/module.go (3)
3-14: Consider organizing imports according to project standards.The imports should be organized using
gciwith the following groups: standard, default.🧰 Tools
🪛 GitHub Check: Linter
[failure] 7-7:
File is notgci-ed with --skip-generated -s standard -s default (gci)
93-93: Fix typos in function comment.Correct the typos:
- "cashe" → "cache"
- "assoseted" → "associated"
104-108: Consider improving error handling.The error handling could be more informative by:
- Using a specific error type for "not found" cases
- Including the IP in the error message for better debugging
- if !errors.Is(err, database.ErrNotFound) { - log.Errorf("nameserver: failed to search for IP info record: %s", err) + if !errors.Is(err, database.ErrNotFound) { + log.Errorf("nameserver: failed to search for IP info record %s: %s", ipString, err)service/firewall/interception/dnsmonitor/eventlistener_linux.go (4)
6-14: Organize imports according to standard conventions.The imports should be organized into groups: standard library, external dependencies, and internal packages.
Apply this ordering:
import ( "errors" "fmt" "net" "os" - "github.com/safing/portmaster/service/mgr" - "github.com/varlink/go/varlink" + "github.com/varlink/go/varlink" + + "github.com/safing/portmaster/service/mgr" )🧰 Tools
🪛 GitHub Check: Linter
[failure] 12-12:
File is notgci-ed with --skip-generated -s standard -s default (gci)
[failure] 13-13:
File is notgci-ed with --skip-generated -s standard -s default (gci)
29-35: Define retry attempts limit as a constant.The magic number 10 for retry attempts should be defined as a package-level constant for better maintainability.
+const maxRestartAttempts = 10 + func newListener(module *DNSMonitor) (*Listener, error) { // ... - if restartAttempts > 10 { + if restartAttempts > maxRestartAttempts {🧰 Tools
🪛 GitHub Check: Linter
[failure] 33-33:
Magic number: 10, in detected (mnd)
99-100: Define slice capacities as constants.The magic number 5 for initial IP slice capacity should be defined as a constant.
+const defaultIPCapacity = 5 + -ips := make([]net.IP, 0, 5) +ips := make([]net.IP, 0, defaultIPCapacity)🧰 Tools
🪛 GitHub Check: Linter
[failure] 100-100:
Magic number: 5, in detected (mnd)
97-123: Consider optimizing domain processing.The domain processing logic could be improved:
- Consider pre-allocating the cnames map with an initial size
- Add validation for domain name format
func (l *Listener) processAnswer(queryResult *QueryResult) { + // Validate domain format + if !isValidDomain((*queryResult.Question)[0].Name) { + return + } + // Allocated data struct for the parsed result. - cnames := make(map[string]string) + cnames := make(map[string]string, len(*queryResult.Answer))🧰 Tools
🪛 GitHub Check: Linter
[failure] 100-100:
Magic number: 5, in detected (mnd)service/firewall/interception/windowskext/packet.go (1)
Line range hint
62-67: Consider handling of non-Layer3 packetsSince we're now explicitly using
ParseLayer3, we should ensure that:
- The Windows kernel extension always provides Layer 3 packets
- Error handling properly identifies and reports layer-specific parsing failures
Consider adding a comment documenting that this code expects Layer 3 packets from the Windows kernel extension, or implement layer detection if needed.
service/network/packet/parse.go (2)
Line range hint
109-158: Enhance function documentation with parameter descriptions.The function implementation looks good, but the documentation could be more descriptive.
Consider expanding the documentation to include:
-// ParseLayer3 parses an IP packet and saves the information in the given packet object. +// ParseLayer3 parses an IP packet (IPv4 or IPv6) and populates the Base structure with +// the parsed information. +// +// Parameters: +// - packetData: Raw bytes of the IP packet to parse +// - pktBase: Pointer to Base structure to store parsed information +// +// Returns: +// - error: Parsing errors or nil on success
159-160: Enhance function documentation.The function documentation could be more descriptive about its purpose and requirements.
Consider expanding the documentation:
-// ParseLayer4 parses an layer 4 packet and saves the information in the given packet object. +// ParseLayer4 parses a transport layer (Layer 4) packet based on the protocol +// specified in pktBase.info.Protocol. The function expects the protocol to be +// already set by a previous Layer 3 parsing operation. +// +// Parameters: +// - packetData: Raw bytes of the transport layer packet +// - pktBase: Pointer to Base structure containing protocol information and for storing results +// +// Returns: +// - error: Parsing errors or nil on successservice/firewall/interception/windowskext2/handler.go (1)
Line range hint
124-127: Add payloadLayer field to IPv6 packet handling.The
payloadLayerfield is missing in the IPv6 packet creation, which could lead to inconsistent packet parsing behavior between IPv4 and IPv6.Apply this diff to maintain consistency:
newPacket := &Packet{ verdictRequest: conn.ID, payload: conn.Payload, + payloadLayer: conn.PayloadLayer, verdictSet: abool.NewBool(false), }service/resolver/ipinfo.go (1)
55-74: Consider improving the implementation with constants and better documentation.The implementation looks correct but could benefit from the following improvements:
- Define the magic number 50 as a constant to improve maintainability
- Add documentation about the cycle detection behavior
- Consider adding debug logging for cycle detection
Here's a suggested improvement:
+// maxCNAMEChainLength defines the maximum number of CNAME records to follow +// to prevent infinite loops in circular CNAME chains. +const maxCNAMEChainLength = 50 + // AddCNAMEs adds all cnames from the map related to its set Domain. +// It follows CNAME chains up to maxCNAMEChainLength records deep. +// The method stops when: +// - A non-CNAME record is encountered +// - A cycle is detected (CNAME points to original domain or itself) +// - The maximum chain length is reached func (resolved *ResolvedDomain) AddCNAMEs(cnames map[string]string) { // Resolve all CNAMEs in the correct order and add the to the record - up to max 50 layers. domain := resolved.Domain domainLoop: - for range 50 { + for range maxCNAMEChainLength { nextDomain, isCNAME := cnames[domain] switch { case !isCNAME:service/firewall/interception/nfq/nfq.go (1)
Line range hint
191-195: Consider enhancing error handling for specific parse failures.Currently, all parse errors result in packet drops. Consider categorizing parse errors to handle recoverable cases differently.
- if err := pmpacket.ParseLayer3(*attrs.Payload, &pkt.Base); err != nil { - log.Warningf("nfqueue: failed to parse payload: %s", err) - _ = pkt.Drop() - return 0 - } + if err := pmpacket.ParseLayer3(*attrs.Payload, &pkt.Base); err != nil { + // Categorize parse errors + switch { + case errors.Is(err, pmpacket.ErrInvalidPacket): + log.Warningf("nfqueue: invalid packet format: %s", err) + _ = pkt.Drop() + case errors.Is(err, pmpacket.ErrUnsupportedProtocol): + log.Debugf("nfqueue: unsupported protocol: %s", err) + _ = pkt.Accept() + default: + log.Warningf("nfqueue: failed to parse payload: %s", err) + _ = pkt.Drop() + } + return 0 + }service/firewall/dns.go (3)
305-308: Consider enhancing the function documentation.While the function is well-implemented, the comment could be more descriptive about what "friendlier" means in this context.
Suggested documentation:
-// formatRR is a friendlier alternative to miekg/dns.RR.String(). +// formatRR formats DNS resource records by replacing tabs with spaces for improved +// readability in logs and error messages. It provides a more readable alternative +// to miekg/dns.RR.String().
310-311: Fix typos in function documentation.The function comment contains several typos:
- "cashe" should be "cache"
- "assoseted" should be "associated"
-// SaveIPsInCache saves the provided ips in the dns cashe assoseted with the record Domain and CNAMEs. +// SaveIPsInCache saves the provided IPs in the DNS cache associated with the record Domain and CNAMEs.
Line range hint
310-339: Improve error handling and add input validation.Consider the following improvements:
- Add validation for input parameters
- Provide more specific error messages
- Consider logging ErrNotFound at debug level for better debugging
func SaveIPsInCache(ips []net.IP, profileID string, record resolver.ResolvedDomain) { + // Validate input parameters + if len(profileID) == 0 { + log.Error("nameserver: empty profileID provided to SaveIPsInCache") + return + } + // Package IPs and CNAMEs into IPInfo structs. for _, ip := range ips { // Never save domain attributions for localhost IPs. if netutils.GetIPScope(ip) == netutils.HostLocal { continue } ipString := ip.String() info, err := resolver.GetIPInfo(profileID, ipString) if err != nil { if !errors.Is(err, database.ErrNotFound) { - log.Errorf("nameserver: failed to search for IP info record: %s", err) + log.Errorf("nameserver: failed to search for IP info record for IP %s: %s", ipString, err) + } else { + log.Debugf("nameserver: no existing IP info record found for IP %s", ipString) } info = &resolver.IPInfo{ IP: ipString, ProfileID: profileID, } } // Add the new record to the resolved domains for this IP and scope. info.AddDomain(record) // Save if the record is new or has been updated. if err := info.Save(); err != nil { - log.Errorf("nameserver: failed to save IP info record: %s", err) + log.Errorf("nameserver: failed to save IP info record for IP %s: %s", ipString, err) } } }service/firewall/packet_handler.go (3)
449-451: Consider extracting DNS interception conditions into a helper function.The complex condition for DNS query interception could be more readable if extracted into a helper function.
+func shouldInterceptDNSQuery(conn *network.Connection, pkt packet.Packet) bool { + return dnsQueryInterception() && + !module.instance.Resolver().IsDisabled() && + pkt.IsOutbound() && + pkt.Info().DstPort == 53 && + nameserverIPMatcherReady.IsSet() && + !nameserverIPMatcher(pkt.Info().Dst) && + !(conn.Process().Pid == ownPID && + conn.Entity.IPScope == netutils.LocalMulticast) +} case dnsQueryInterception() && - !module.instance.Resolver().IsDisabled() && - pkt.IsOutbound() && - pkt.Info().DstPort == 53 && - nameserverIPMatcherReady.IsSet() && - !nameserverIPMatcher(pkt.Info().Dst) && - !(conn.Process().Pid == ownPID && - conn.Entity.IPScope == netutils.LocalMulticast): + shouldInterceptDNSQuery(conn, pkt):
484-486: Add documentation for DNS inspection logic.The purpose and behavior of DNS inspection should be documented for better maintainability.
+// If the connection is marked for inspection and looks like an outgoing DNS request, +// parse and validate the DNS packet, then update the DNS cache accordingly. case conn.Inspecting && looksLikeOutgoingDNSRequest(conn): inspectDNSPacket(conn, pkt) conn.UpdateFirewallHandler(inspectDNSPacket)
619-626: Consider a more descriptive resolver name.The current name "Direct DNS request" could be more specific about the nature of the resolver.
Would you like me to suggest alternative names that better describe the resolver's purpose? Some options could include:
- "External DNS Request"
- "Non-Portmaster DNS Request"
- "Third-party DNS Request"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.golangci.yml(0 hunks)go.mod(2 hunks)service/firewall/bypassing.go(2 hunks)service/firewall/dns.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_linux.go(1 hunks)service/firewall/interception/dnsmonitor/module.go(1 hunks)service/firewall/interception/nfq/nfq.go(1 hunks)service/firewall/interception/windowskext/packet.go(1 hunks)service/firewall/interception/windowskext2/handler.go(1 hunks)service/firewall/interception/windowskext2/packet.go(3 hunks)service/firewall/packet_handler.go(6 hunks)service/instance.go(9 hunks)service/network/packet/parse.go(2 hunks)service/resolver/ipinfo.go(1 hunks)service/resolver/resolver.go(2 hunks)
💤 Files with no reviewable changes (1)
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🪛 GitHub Check: Linter
service/firewall/interception/dnsmonitor/eventlistener_linux.go
[failure] 12-12:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 13-13:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 100-100:
Magic number: 5, in detected (mnd)
[failure] 33-33:
Magic number: 10, in detected (mnd)
service/firewall/interception/dnsmonitor/module.go
[failure] 7-7:
File is not gci-ed with --skip-generated -s standard -s default (gci)
service/firewall/packet_handler.go
[failure] 595-595:
Error return value of pkt.LoadPacketData is not checked (errcheck)
[failure] 600-600:
Error return value of pkt.PermanentBlock is not checked (errcheck)
[failure] 606-606:
Error return value of pkt.Accept is not checked (errcheck)
service/network/packet/parse.go
[failure] 166-166:
missing cases in switch of type packet.IPProtocol: packet.UDPLite, packet.RAW, packet.AnyHostInternalProtocol61 (exhaustive)
🪛 golangci-lint (1.61.0)
service/firewall/packet_handler.go
595-595: Error return value of pkt.LoadPacketData is not checked
(errcheck)
🔇 Additional comments (23)
service/firewall/interception/windowskext2/packet.go (1)
23-23: Verify 'payloadLayer' is properly initialized before use
Ensure that the new field payloadLayer is correctly initialized before calling LoadPacketData to prevent potential parsing errors.
Run the following script to check where payloadLayer is set:
Expect to find assignments initializing payloadLayer before LoadPacketData is called.
service/firewall/bypassing.go (1)
Line range hint 46-94: Verify integration with resolver module and existing DNS handling
Let's verify the integration points and potential impact.
✅ Verification successful
Integration with resolver module and DNS handling is correctly implemented
The code changes show proper integration with the resolver module and DNS handling:
- The resolver's
IsDisabled()check is correctly implemented and used - System resolver detection via
IsSystemResolver()is properly integrated - DNS packet inspection is implemented with appropriate validation and blocking
- The bypass prevention logic correctly handles different DNS scenarios (UDP/53, DoT/853)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify resolver module integration and DNS handling
# Check resolver module interface implementation
ast-grep --pattern 'type $_ interface {
$$$
Resolver() $_
$$$
}'
# Check DNS packet inspection implementation
ast-grep --pattern 'func inspectDNSPacket($_) $_ {
$$$
}'
# Look for other DNS handling code that might be affected
rg -l "DNS|dns|port.*53|resolver" --type go
Length of output: 4215
Script:
#!/bin/bash
# Check DNS packet inspection implementation in firewall
rg -A 10 "looksLikeOutgoingDNSRequest" --type go
# Check resolver module's IsDisabled implementation
ast-grep --pattern 'func (*$_) IsDisabled() bool {
$$$
}'
# Check system resolver detection
ast-grep --pattern 'func (*$_) IsSystemResolver() bool {
$$$
}'
Length of output: 2307
Script:
#!/bin/bash
# Check DNS packet inspection implementation
rg -A 15 "func inspectDNSPacket" --type go
# Check resolver module's IsDisabled implementation in resolver package
rg -A 5 "IsDisabled" service/resolver/
# Check system resolver detection in process package
rg -A 5 "IsSystemResolver" service/process/
Length of output: 1890
service/firewall/interception/dnsmonitor/module.go (6)
16-20: LGTM! Well-defined resolver information.
The resolver information is properly structured with clear metadata.
34-44: Return error when listener initialization fails.
The Start() method swallows the error from listener initialization, which could hide important startup issues.
46-55: Return error when listener shutdown fails.
The Stop() method should propagate errors from listener shutdown to allow proper error handling by callers.
57-61: Add nil check for listener in Flush().
The Flush() method should check if the listener exists before calling flush to prevent nil pointer dereference.
79-91: LGTM! Clean constructor and interface design.
The constructor and interface are well-designed with clear responsibilities.
65-70: 🛠️ Refactor suggestion
Consider setting an appropriate expiration time for resolved domains.
Setting Expires to 0 might lead to stale records persisting indefinitely. Consider implementing a TTL-based expiration strategy.
service/firewall/interception/dnsmonitor/eventlistener_linux.go (2)
16-18: LGTM!
The Listener struct is well-defined with a clear purpose.
39-42: Use module.Ctx() instead of module.mgr.Ctx().
As noted in previous reviews, the context should be obtained directly from the module.
-varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor")
+varlinkConn, err := varlink.NewConnection(module.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor")service/firewall/interception/windowskext/packet.go (2)
Line range hint 62-67: LGTM! Error handling and logging look solid
The implementation maintains proper error handling with context-aware logging and appropriate error propagation.
Line range hint 62-67: Verify layer-specific packet parsing implementation
The change from Parse to ParseLayer3 appears to be part of a broader refactoring to make packet layer parsing more explicit. Let's verify the implementation is consistent across the codebase.
✅ Verification successful
Layer-specific packet parsing implementation is consistent
The verification shows that:
- No legacy
packet.Parse()calls remain in the codebase ParseLayer3andParseLayer4are properly implemented inservice/network/packet/parse.go- The implementation is consistently used across different packet handlers:
windowskext/packet.go(the file under review)nfq/nfq.gowindowskext2/packet.go(which also handles Layer4)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ParseLayer3 implementation and its usage
# Check for any remaining Parse calls that should be updated
rg "packet\.Parse\(" --type go
# Check the ParseLayer3 implementation
ast-grep --pattern 'func ParseLayer3($$$) {
$$$
}'
# Look for similar packet parsing implementations
rg "ParseLayer[34]" -A 5 --type go
Length of output: 2710
service/firewall/interception/windowskext2/handler.go (1)
58-58: LGTM, but verify IPv6 handling consistency.
The addition of payloadLayer field is good, as it provides additional information for packet parsing.
Let's verify if IPv6 packets also need this field by checking the packet parsing logic:
service/resolver/ipinfo.go (1)
55-74: Verify the usage of AddCNAMEs method.
Let's ensure this method is being used correctly throughout the codebase.
✅ Verification successful
Let me gather more context about the ResolvedDomain struct and its usage.
Based on the comprehensive results, I can now provide a final response as the verification is complete.
AddCNAMEs method is correctly integrated and used.
The method is properly used in two critical places:
- In
service/firewall/dns.gofor processing DNS responses - In
service/firewall/interception/dnsmonitor/module.gofor DNS monitoring
The implementation correctly:
- Integrates with the
ResolvedDomainstruct which is part of the core DNS resolution system - Is used consistently with the DNS monitoring and firewall subsystems
- Maintains proper state management of CNAMEs in the DNS resolution chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how AddCNAMEs is being used in the codebase
# Find all direct calls to AddCNAMEs
echo "=== Direct calls to AddCNAMEs ==="
rg -A 3 "AddCNAMEs\("
# Find all places where ResolvedDomain is used to understand the context
echo "=== ResolvedDomain usage ==="
ast-grep --pattern 'type $_ struct {
$$$
ResolvedDomain
$$$
}'
Length of output: 1057
Script:
#!/bin/bash
# Find the ResolvedDomain struct definition and usage
echo "=== ResolvedDomain struct definition ==="
ast-grep --pattern 'type ResolvedDomain struct {
$$$
}'
# Find where ResolvedDomain is used as a type
echo "=== ResolvedDomain type usage ==="
rg "ResolvedDomain" -A 2 -B 2
# Check the DNS monitor module implementation
echo "=== DNS Monitor implementation ==="
rg "func.*DNS" -A 5
Length of output: 30898
service/resolver/resolver.go (2)
31-31: LGTM: New resolver source constant follows conventions.
The addition of ServerSourcePacket maintains consistency with existing source constants and naming conventions.
86-86: Verify packet resolver implementation.
The documentation indicates support for packet-based resolution. Let's verify the implementation of this new resolver type.
service/firewall/interception/nfq/nfq.go (1)
Line range hint 191-195: Verify packet parsing changes across the codebase.
The switch from Parse to ParseLayer3 suggests a more layered approach to packet parsing. While this change improves modularity, we should ensure all necessary packet information is still captured.
Let's verify the usage and impact:
✅ Verification successful
Let me gather more information about the packet parsing changes to understand the full context.
Let me try one final verification to understand the packet parsing changes better.
Packet parsing changes are correctly implemented
The switch to ParseLayer3 is a well-structured change that:
- Properly handles layer 3 packet parsing with appropriate error checking
- Maintains the necessary packet information in the
Basestruct - Is consistently implemented across the codebase, only used in the NFQueue handling
- Correctly drops malformed packets, which is the appropriate behavior for network security
The error handling with pkt.Drop() is the right approach as it prevents malformed packets from proceeding further in the network stack.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other uses of packet parsing to ensure consistency
# Check for other Parse/ParseLayer3 usages
rg -A 3 'Parse(Layer3)?\(' 'service/network/packet'
# Check for potential packet drops in error paths
ast-grep --pattern 'if err := pmpacket.ParseLayer3($_, $_); err != nil {
$$$
}'
Length of output: 788
Script:
#!/bin/bash
# Check for the old Parse function to understand the transition
rg -A 5 'func Parse\(' 'service/network/packet'
# Look for other packet handling implementations
ast-grep --pattern 'type $_ struct {
Base $$$
}'
# Check for any tests related to packet parsing
fd -e go -E 'vendor/*' --exec rg -l 'Test.*Parse'
Length of output: 223
Script:
#!/bin/bash
# Check the content of the ParseLayer3 function
rg -B 2 -A 10 'func ParseLayer3' 'service/network/packet'
# Look for any related packet handling types and interfaces
rg -A 5 'type (Packet|Base)' 'service/network/packet'
# Check for any other files that might be using packet parsing
rg --type go -l 'pmpacket\.' -g '!vendor/'
Length of output: 1645
service/firewall/dns.go (1)
290-303: LGTM! Well-structured record creation and CNAME processing.
The code demonstrates good separation of concerns by:
- Creating a complete record with all necessary context
- Properly handling CNAME processing
- Delegating IP caching to a dedicated function
service/instance.go (5)
22-23: LGTM: Import additions are well-organized
The new imports for dnsmonitor and integration packages are correctly placed within their respective groups, maintaining the project's import organization pattern.
70-70: LGTM: Struct field additions are well-organized
The new fields integration and dnsmonitor are:
- Correctly placed within their respective groups in the struct
- Follow the project's naming conventions
- Properly typed with qualified package names
Also applies to: 80-80
394-397: LGTM: Getter methods follow established patterns
The new getter methods OSIntegration() and DNSMonitor() are:
- Well-documented
- Follow the project's naming conventions
- Consistently implemented
Also applies to: 484-487
157-160: Verify module initialization dependencies
The initialization and service group registration of the new modules follow the established pattern. However, let's verify there are no circular dependencies or initialization order requirements between these modules.
Also applies to: 197-200, 289-289, 303-303
✅ Verification successful
Let me gather more information about the dependency relationship by checking the actual usage in the files where imports were found.
Based on the gathered information, I can now provide the final response:
Module initialization order is correct and dependencies are properly managed
The analysis reveals:
- The integration module is initialized before dnsmonitor in
instance.go - DNSMonitor only uses integration through a clean interface dependency (
OSIntegration()) and ETW-specific functionality on Windows - There are no circular dependencies between the modules
- Both modules follow the established initialization pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular dependencies between modules
# Look for imports of integration in dnsmonitor and vice versa
echo "Checking for potential circular dependencies..."
rg -l "github.com/safing/portmaster/service/integration" service/firewall/interception/dnsmonitor/
rg -l "github.com/safing/portmaster/service/firewall/interception/dnsmonitor" service/integration/
echo "Checking for initialization order requirements..."
# Look for usage of these modules in each other's New functions
rg -A 5 "func New\(" service/integration/
rg -A 5 "func New\(" service/firewall/interception/dnsmonitor/
Length of output: 1432
Script:
#!/bin/bash
# Check how integration module is used in dnsmonitor files
echo "=== Usage of integration in dnsmonitor/module.go ==="
rg -C 5 "integration" service/firewall/interception/dnsmonitor/module.go
echo -e "\n=== Usage of integration in dnsmonitor/etwlink_windows.go ==="
rg -C 5 "integration" service/firewall/interception/dnsmonitor/etwlink_windows.go
# Check the initialization order in instance.go
echo -e "\n=== Module initialization sequence in instance.go ==="
rg -C 2 "New\(" service/instance.go
Length of output: 7233
Line range hint 601-626: Verify proper cleanup during shutdown
While the modules are properly initialized and integrated, let's verify they implement proper cleanup during shutdown.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the cleanup implementations.
Proper cleanup is implemented in both modules
The verification shows that both modules implement proper cleanup during shutdown:
OSIntegrationmodule implementsStop()which callsCleanUp()DNSMonitormodule implementsStop()which properly closes the listener- Windows-specific ETW components have proper cleanup with
StopTrace(),DestroySession(), andStopOldSession()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new modules implement cleanup methods
echo "Checking for cleanup implementations in integration module..."
rg -A 5 "func.*Stop|func.*Close|func.*Cleanup" service/integration/
echo "Checking for cleanup implementations in dnsmonitor module..."
rg -A 5 "func.*Stop|func.*Close|func.*Cleanup" service/firewall/interception/dnsmonitor/
echo "Checking if modules implement mgr.Module interface..."
ast-grep --pattern 'type $_ struct {
$$$
}
func ($v $_) Start() error {
$$$
}
func ($v $_) Stop() error {
$$$
}'
Length of output: 3852
| err = packet.ParseLayer4(pkt.payload, &pkt.Base) | ||
| default: | ||
| err = fmt.Errorf("unsupported payload layer: %d", pkt.payloadLayer) | ||
| } | ||
| if err != nil { | ||
| log.Tracef("payload: %#v", pkt.payload) |
There was a problem hiding this comment.
Avoid logging raw payload data to prevent sensitive information exposure
Logging the raw payload with log.Tracef("payload: %#v", pkt.payload) may expose sensitive data contained in the packet payload. Consider removing this log statement or ensuring sensitive data is not exposed.
Apply this diff to remove the logging of raw payload:
- log.Tracef("payload: %#v", pkt.payload)If detailed logging is needed for debugging, ensure that any sensitive data is appropriately sanitized or masked.
📝 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.
| log.Tracef("payload: %#v", pkt.payload) |
| case looksLikeOutgoingDNSRequest(conn) && module.instance.Resolver().IsDisabled(): | ||
| // Allow. Packet will be analyzed and blocked if its not a dns request, before sent. | ||
| conn.Inspecting = true | ||
| return endpoints.NoMatch, "", nil |
There was a problem hiding this comment.
Security: Enhance DNS inspection mechanism
The current implementation allows packets before inspection completes, which could create a small window of opportunity for attacks.
Consider these improvements:
- Buffer initial packets until inspection confirms DNS validity
- Add timeout for inspection to prevent hanging connections
- Consider implementing pre-inspection validation
case looksLikeOutgoingDNSRequest(conn) && module.instance.Resolver().IsDisabled():
- // Allow. Packet will be analyzed and blocked if its not a dns request, before sent.
conn.Inspecting = true
+ conn.SetInspectionTimeout(time.Second * 5) // Add timeout
+ conn.BufferInitialPackets = true // Buffer until inspection
return endpoints.NoMatch, "", nilCommittable suggestion skipped: line range outside the PR's diff.
service/firewall/interception/dnsmonitor/eventlistener_linux.go
Outdated
Show resolved
Hide resolved
| availableDecoders := []func(gopacket.Packet, *Info) error{ | ||
| parseTCP, | ||
| parseUDP, | ||
| // parseUDPLite, // We don't yet support udplite. | ||
| parseICMPv4, | ||
| parseICMPv6, | ||
| parseIGMP, | ||
| checkError, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize decoder selection based on protocol.
The function currently attempts all decoders regardless of the protocol type. This is inefficient and could potentially lead to incorrect parsing.
Consider selecting only the relevant decoder based on the protocol:
-availableDecoders := []func(gopacket.Packet, *Info) error{
- parseTCP,
- parseUDP,
- // parseUDPLite, // We don't yet support udplite.
- parseICMPv4,
- parseICMPv6,
- parseIGMP,
- checkError,
-}
+var decoder func(gopacket.Packet, *Info) error
+switch pktBase.info.Protocol {
+case TCP:
+ decoder = parseTCP
+case UDP:
+ decoder = parseUDP
+case ICMP:
+ decoder = parseICMPv4
+case ICMPv6:
+ decoder = parseICMPv6
+case IGMP:
+ decoder = parseIGMP
+default:
+ decoder = checkError
+}Then update the decoder execution:
-for _, dec := range availableDecoders {
- if err := dec(packet, pktBase.Info()); err != nil {
- return err
- }
-}
+if err := decoder(packet, pktBase.Info()); err != nil {
+ return err
+}Committable suggestion skipped: line range outside the PR's diff.
| // Read create structs with the needed data. | ||
| question := dnsPacket.Question[0] | ||
| fqdn := dns.Fqdn(question.Name) |
There was a problem hiding this comment.
Add bounds check for DNS question array.
The code assumes that the question array has at least one element without checking.
-// Read create structs with the needed data.
-question := dnsPacket.Question[0]
-fqdn := dns.Fqdn(question.Name)
+// Read and create structs with the needed data.
+if len(dnsPacket.Question) == 0 {
+ log.Tracer(pkt.Ctx()).Debugf("filter: DNS packet has no questions")
+ return
+}
+question := dnsPacket.Question[0]
+fqdn := dns.Fqdn(question.Name)📝 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.
| // Read create structs with the needed data. | |
| question := dnsPacket.Question[0] | |
| fqdn := dns.Fqdn(question.Name) | |
| // Read and create structs with the needed data. | |
| if len(dnsPacket.Question) == 0 { | |
| log.Tracer(pkt.Ctx()).Debugf("filter: DNS packet has no questions") | |
| return | |
| } | |
| question := dnsPacket.Question[0] | |
| fqdn := dns.Fqdn(question.Name) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
service/firewall/interception/dnsmonitor/module.go (2)
99-99: Handle error from SubmitDNSCheckDomainThe error from
SubmitDNSCheckDomainis being discarded. Consider logging the error for debugging purposes.- _ = compat.SubmitDNSCheckDomain(subdomain) + if err := compat.SubmitDNSCheckDomain(subdomain); err != nil { + log.Debugf("dnsmonitor: failed to submit DNS check domain: %s", err) + }
108-108: Fix typos in function commentThe comment has typos: "cashe" should be "cache" and "assoseted" should be "associated".
-// saveIPsInCache saves the provided ips in the dns cashe assoseted with the record Domain and CNAMEs. +// saveIPsInCache saves the provided IPs in the DNS cache associated with the record Domain and CNAMEs.service/firewall/interception/dnsmonitor/eventlistener_linux.go (5)
18-20: Add documentation for the Listener struct.Consider adding a doc comment explaining the purpose of the Listener struct and its role in DNS monitoring.
+// Listener implements DNS monitoring for Linux systems using systemd-resolver's +// varlink interface to capture DNS query events. type Listener struct { varlinkConn *varlink.Connection }
45-52: Improve connection cleanup logic.The current defer cleanup is nested within the worker function. Consider moving it to the Listener struct for better resource management.
+func (l *Listener) closeConnection() { + if l.varlinkConn != nil { + if err := l.varlinkConn.Close(); err != nil { + log.Errorf("dnsmonitor: failed to close varlink connection: %s", err) + } + l.varlinkConn = nil + } +} func newListener(module *DNSMonitor) (*Listener, error) { // ... existing code ... module.mgr.Go("systemd-resolver-event-listener", func(w *mgr.WorkerCtx) error { - defer func() { - if varlinkConn != nil { - err = varlinkConn.Close() - if err != nil { - log.Errorf("dnsmonitor: failed to close varlink connection: %s", err) - } - } - }() + defer listener.closeConnection()
107-114: Document empty methods.Add documentation explaining why these methods are empty and their purpose in the interface implementation.
+// flush implements the Listener interface but has no operation for Linux systems +// as the systemd resolver doesn't require explicit flushing. func (l *Listener) flush() error { // Nothing to flush return nil } +// stop implements the Listener interface but has no operation for Linux systems +// as connection cleanup is handled by the worker context. func (l *Listener) stop() error { return nil }
119-119: Define capacity magic number as a constant.The initial capacity of 5 for the IP slice should be defined as a named constant.
+const defaultIPsCapacity = 5 -ips := make([]net.IP, 0, 5) +ips := make([]net.IP, 0, defaultIPsCapacity)🧰 Tools
🪛 GitHub Check: Linter
[failure] 119-119:
Magic number: 5, in detected (mnd)
126-126: Fix typo in comment.Correct the spelling of "trough" to "through".
-// Go trough each answer entry. +// Go through each answer entry.service/compat/selfcheck.go (1)
161-166: Add logging for skipped resolver check.The resolver check logic is correct, but consider adding debug logging when skipping the check to aid in troubleshooting.
Consider adding a debug log before returning:
if module.instance.Resolver().IsDisabled() { + log.Tracer(ctx).Debugf("compat: self-check #3: dns integration response check skipped: resolver is disabled") // There is no control over the response, there is nothing more that can be checked. return nil, nil }service/firewall/packet_handler.go (1)
634-635: Address the TODO comment forresolverInfo.NameThere is a TODO comment indicating a need for a better name for the resolver in the
resolverInfostruct. Providing a more descriptive name can enhance code readability and maintainability. Consider naming it based on the DNS request's context, such as"External DNS Request"or"Direct DNS Resolver".Would you like assistance in suggesting a more appropriate name for the resolver?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
service/compat/module.go(1 hunks)service/compat/selfcheck.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_linux.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_windows.go(1 hunks)service/firewall/interception/dnsmonitor/module.go(1 hunks)service/firewall/packet_handler.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- service/firewall/interception/dnsmonitor/eventlistener_windows.go
🧰 Additional context used
🪛 GitHub Check: Linter
service/firewall/interception/dnsmonitor/eventlistener_linux.go
[failure] 12-12:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 15-15:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 119-119:
Magic number: 5, in detected (mnd)
[failure] 35-35:
Magic number: 10, in detected (mnd)
service/firewall/interception/dnsmonitor/module.go
[failure] 8-8:
File is not gci-ed with --skip-generated -s standard -s default (gci)
service/firewall/packet_handler.go
[failure] 596-596:
Error return value of pkt.LoadPacketData is not checked (errcheck)
[failure] 601-601:
Error return value of pkt.PermanentBlock is not checked (errcheck)
[failure] 607-607:
Error return value of pkt.Accept is not checked (errcheck)
🪛 golangci-lint (1.61.0)
service/firewall/packet_handler.go
596-596: Error return value of pkt.LoadPacketData is not checked
(errcheck)
🔇 Additional comments (5)
service/firewall/interception/dnsmonitor/module.go (1)
37-46:
Improve error handling in module lifecycle methods
The following critical issues with error handling need to be addressed:
Start(): Returns nil even when listener initialization failsStop(): Swallows errors from listener shutdownFlush(): Missing nil check for listener
The issues were previously identified in past reviews. Please refer to those comments for the suggested fixes.
Also applies to: 49-57, 60-62
service/firewall/interception/dnsmonitor/eventlistener_linux.go (1)
41-42: Use module.Ctx() instead of module.mgr.Ctx().
As suggested in the past review, the context should be obtained directly from the module.
-varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor")
+varlinkConn, err := varlink.NewConnection(module.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor")service/compat/module.go (2)
184-184: Missing implementation in spn/instance.go
As identified in the previous review, the spn/instance.go file needs to implement the newly added Resolver() method to maintain interface compatibility.
184-184: LGTM! Verify interface implementations.
The addition of Resolver() method to the interface is clean and follows Go interface design patterns.
Let's verify all implementations of this interface:
✅ Verification successful
Let me try a different search pattern to find implementations, as the previous one might have been too strict.
Let me search for the actual implementation of the Resolver method.
Based on the findings, I can now generate the final response:
Implementation verified and consistent
The Resolver() method is properly implemented in service/instance.go and returns the resolver module as required by the interface. The implementation is consistent with the interface definition and follows the same pattern as other module accessor methods in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all types implementing the instance interface
# Expected: All implementations should have both NetEnv() and Resolver() methods
# Search for struct types implementing NetEnv()
echo "=== Types implementing NetEnv() ==="
ast-grep --pattern 'func ($v $_) NetEnv() *netenv.NetEnv {
$$$
}'
# Search for struct types implementing Resolver()
echo "=== Types implementing Resolver() ==="
ast-grep --pattern 'func ($v $_) Resolver() *resolver.ResolverModule {
$$$
}'
Length of output: 317
Script:
#!/bin/bash
# Search for files containing both "instance" and "Resolver" to find potential implementations
echo "=== Files potentially containing instance implementations ==="
rg -l "instance" | xargs rg -l "Resolver"
# Let's also check the content of spn/instance.go since it was mentioned in the past review
echo -e "\n=== Content of spn/instance.go if it exists ==="
fd "instance.go" -t f --exec cat {}
Length of output: 30349
Script:
#!/bin/bash
# Search for the Resolver method implementation in service/instance.go
echo "=== Searching for Resolver method implementation ==="
rg -A 2 "func.*Resolver\(\)" service/instance.go
# Also check if there are any other files that might contain the implementation
echo -e "\n=== Checking other potential implementation locations ==="
rg -l "func.*Resolver\(\)"
Length of output: 483
service/firewall/packet_handler.go (1)
515-515: Confirm the intentional commenting out of conn.Inspecting = false
By commenting out conn.Inspecting = false, the Inspecting flag remains unchanged. Please verify whether this is intentional, as it may affect the inspection logic and could lead to unintended behavior in the connection handling process.
If this change is deliberate, consider removing the commented code to improve code clarity.
| record := resolver.ResolvedDomain{ | ||
| Domain: fqdn, | ||
| Resolver: &ResolverInfo, | ||
| DNSRequestContext: &resolver.DNSRequestContext{}, | ||
| Expires: 0, | ||
| } |
There was a problem hiding this comment.
Set appropriate expiration time for resolved domain records
The Expires field is set to 0, which could cause records to expire immediately. Consider setting an appropriate TTL value based on the DNS response or a default caching duration.
service/firewall/interception/dnsmonitor/eventlistener_linux.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
service/network/packet/parse.go (2)
177-184: Improve error messages for unsupported protocols.The error messages for unsupported protocols could be more descriptive to help with debugging.
- return fmt.Errorf("UDPLite not supported") + return fmt.Errorf("protocol UDPLite (protocol=%d) is not yet supported", pktBase.info.Protocol) - return fmt.Errorf("RAW protocol not supported") + return fmt.Errorf("RAW protocol (protocol=%d) is not supported in layer 4 parsing", pktBase.info.Protocol) - return fmt.Errorf("AnyHostInternalProtocol61 protocol not supported") + return fmt.Errorf("AnyHostInternalProtocol61 (protocol=%d) is not supported in layer 4 parsing", pktBase.info.Protocol) - return fmt.Errorf("protocol not supported") + return fmt.Errorf("unsupported protocol type: %d", pktBase.info.Protocol)
159-213: Consider extracting common packet parsing logic.Both
ParseLayer3andParseLayer4share similar packet creation and payload extraction logic. Consider extracting this common functionality into a helper function to improve maintainability and reduce code duplication.+func parsePacket(packetData []byte, layerType gopacket.LayerType, pktBase *Base, decoder func(gopacket.Packet, *Info) error) error { + if len(packetData) == 0 { + return errors.New("empty packet") + } + + packet := gopacket.NewPacket(packetData, layerType, gopacket.DecodeOptions{ + Lazy: true, + NoCopy: true, + }) + + if err := decoder(packet, pktBase.Info()); err != nil { + return err + } + + pktBase.layers = packet + if transport := packet.TransportLayer(); transport != nil { + pktBase.layer5Data = transport.LayerPayload() + } + return nil +}This would simplify both parsing functions and make the code more maintainable.
service/firewall/packet_handler.go (2)
450-452: Improve readability of DNS query interception logic.Consider extracting the long conditional block into a helper function to improve readability and maintainability.
+func shouldInterceptDNSQuery(conn *network.Connection, pkt packet.Packet) bool { + return dnsQueryInterception() && + !module.instance.Resolver().IsDisabled() && + pkt.IsOutbound() && + pkt.Info().DstPort == 53 && + nameserverIPMatcherReady.IsSet() && + !nameserverIPMatcher(pkt.Info().Dst) && + !(conn.Process().Pid == ownPID && + conn.Entity.IPScope == netutils.LocalMulticast) +} + // Redirect outbound DNS packets if enabled, -case dnsQueryInterception() && - !module.instance.Resolver().IsDisabled() && +case shouldInterceptDNSQuery(conn, pkt):
647-654: Address TODO comment about resolver name.The TODO comment about finding a better name for the resolver should be addressed.
Consider using a more descriptive name like "Direct DNS Request Interceptor" or "DNS Packet Inspector" instead of the generic "Direct DNS request".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
service/firewall/packet_handler.go(6 hunks)service/integration/module.go(1 hunks)service/network/packet/parse.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- service/integration/module.go
🔇 Additional comments (3)
service/network/packet/parse.go (3)
109-110: LGTM! Clear and descriptive function renaming.
The renaming from Parse to ParseLayer3 better reflects the function's purpose, and the documentation clearly describes its functionality.
Line range hint 133-158: Consider optimizing decoder selection.
The function still attempts all decoders regardless of the protocol type, which is inefficient. Please refer to the previous review comment about optimizing decoder selection based on protocol type.
192-206: 🛠️ Refactor suggestion
Refactor decoder selection for consistency with layer type.
The function determines the correct layer type in the switch statement but then attempts all decoders regardless. This is inefficient and could potentially lead to incorrect parsing.
- availableDecoders := []func(gopacket.Packet, *Info) error{
- parseTCP,
- parseUDP,
- // parseUDPLite, // We don't yet support udplite.
- parseICMPv4,
- parseICMPv6,
- parseIGMP,
- checkError,
- }
-
- for _, dec := range availableDecoders {
- if err := dec(packet, pktBase.Info()); err != nil {
- return err
- }
- }
+ var decoder func(gopacket.Packet, *Info) error
+ switch pktBase.info.Protocol {
+ case TCP:
+ decoder = parseTCP
+ case UDP:
+ decoder = parseUDP
+ case ICMP:
+ decoder = parseICMPv4
+ case ICMPv6:
+ decoder = parseICMPv6
+ case IGMP:
+ decoder = parseIGMP
+ default:
+ decoder = checkError
+ }
+
+ if err := decoder(packet, pktBase.Info()); err != nil {
+ return err
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/windows-dll.yml (2)
24-25: Fix indentation for consistent YAML formattingThe checkout step should be indented with 6 spaces to match GitHub Actions' recommended style.
steps: - - name: Checkout Repository - uses: actions/checkout@v4 + - name: Checkout Repository + uses: actions/checkout@v4🧰 Tools
🪛 yamllint (1.29.0-1)
[warning] 24-24: wrong indentation: expected 6 but found 4
(indentation)
1-18: Consider adding workflow optimizationsThe workflow configuration could benefit from some optimizations:
- Add concurrency control to prevent redundant builds:
concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true
- Consider adding path exclusions to prevent unnecessary builds:
paths: - 'windows_core_dll/**' + - '!windows_core_dll/**/*.md' + - '!windows_core_dll/**/*.txt'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/windows-dll.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.3)
.github/workflows/windows-dll.yml
28-28: could not parse as YAML: yaml: line 28: mapping values are not allowed in this context
(syntax-check)
🪛 yamllint (1.29.0-1)
.github/workflows/windows-dll.yml
[warning] 24-24: wrong indentation: expected 6 but found 4
(indentation)
[error] 28-28: syntax error: mapping values are not allowed here
(syntax)
🔇 Additional comments (1)
.github/workflows/windows-dll.yml (1)
29-30: Verify build script existence and permissions
Ensure that the build script exists and has the correct permissions.
✅ Verification successful
Build script exists and contains valid PowerShell commands
The build script exists at the correct location and contains valid PowerShell commands for building the DLL:
- Uses
msbuildto build the solution in Release configuration - Verifies the DLL output with
lscommand - Script has read permissions (644) which is sufficient for PowerShell execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if build script exists and is executable
if [ -f "windows_core_dll/build.ps1" ]; then
echo "Build script exists"
ls -l windows_core_dll/build.ps1
else
echo "Error: build.ps1 not found in windows_core_dll/"
exit 1
fi
Length of output: 197
Script:
#!/bin/bash
# Let's check the content of the build script to ensure it's a valid PowerShell script
cat windows_core_dll/build.ps1
Length of output: 126
vlabo
force-pushed
the
feature/systemd-query-events
branch
2 times, most recently
from
d0102d4 to
7eea0ef
Compare
vlabo
force-pushed
the
feature/systemd-query-events
branch
from
7eea0ef to
dff2f34
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/windows-dll.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/windows-dll.yml (1)
3-17: LGTM! Well-structured workflow triggers
The workflow triggers are properly configured to:
- Build on relevant changes to
windows_core_dll/** - Target important branches (master, develop)
- Allow manual triggering via workflow_dispatch
| func newListener(module *DNSMonitor) (*Listener, error) { | ||
| // Check if the system has systemd-resolver. | ||
| _, err := os.Stat("/run/systemd/resolve/io.systemd.Resolve.Monitor") | ||
| if os.IsNotExist(err) { |
There was a problem hiding this comment.
What if err != nil, but is another error than not exists?
| if restartAttempts > 10 { | ||
| return nil | ||
| } | ||
| defer func() { restartAttempts += 1 }() |
There was a problem hiding this comment.
You can just do startAttempts and count immediately instead of using defer.
It does not matter as defers got really cheap in a recent Go release, but it is still simpler to read.
|
|
||
| var ResolverInfo = resolver.ResolverInfo{ | ||
| Name: "SystemdResolver", | ||
| Type: resolver.ServerSourceEnv, |
There was a problem hiding this comment.
Please use a new type and source:
ServerTypeMonitor = "monitor"
ServerSourceMonitor = "monitor"
There was a problem hiding this comment.
You can also use different source, like "systemd", "packet" or "etw". That would be nice. Then it is very clear where something came from.
service/firewall/packet_handler.go
Outdated
| log.Errorf("filter: failed to accept dns packet: %s", err) | ||
| } | ||
|
|
||
| conn.Type = network.DNSRequest |
There was a problem hiding this comment.
The connection technically is still an IP connection. This should not be changed.
The DNSRequest ist only used for actual DNS requests that came in through the nameserver.
As also, the connection can carry multiple DNS requests.
service/firewall/packet_handler.go
Outdated
| } | ||
|
|
||
| // Packet was parsed, accept the connection and continue. | ||
| err = pkt.Accept() |
There was a problem hiding this comment.
I would only accept the packet when we have added all the data to the cache, as otherwise, this creates a race condition.
service/firewall/packet_handler.go
Outdated
| Name: "Direct DNS request", // TODO(vladimir): Better name? | ||
| Type: resolver.ServerTypeDNS, | ||
| Source: resolver.ServerSourcePacket, |
There was a problem hiding this comment.
Please simply use "DNS Monitor" as name and "monitor" as type. Source IDs can be as mentioned before.
There was a problem hiding this comment.
I understand now what you meant here. Let's talk about this.
| @@ -57,6 +57,10 @@ func PreventBypassing(ctx context.Context, conn *network.Connection) (endpoints. | |||
|
|
|||
| // Block bypass attempts using an (encrypted) DNS server. | |||
| switch { | |||
| case looksLikeOutgoingDNSRequest(conn) && module.instance.Resolver().IsDisabled(): | |||
| // Allow. Packet will be analyzed and blocked if its not a dns request, before sent. | |||
| conn.Inspecting = true | |||
There was a problem hiding this comment.
conn.Inspecting is currently unused. Are you using it?
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
.github/workflows/windows-dll.yml (3)
19-23: Fix indentation for better readability.While the job configuration is functionally correct, the YAML indentation should be consistent.
Apply this diff to fix the indentation:
jobs: build: name: Build runs-on: windows-latest - steps: + steps:
30-36: Improve DLL verification with size and timestamp checks.The verification step could be more thorough by checking additional file properties.
Apply this diff to enhance the verification:
- name: Verify DLL shell: powershell run: | - if (!(Test-Path "windows_core_dll/x64/Release/portmaster-core.dll")) { - Write-Error "DLL build failed: portmaster-core.dll not found" - exit 1 - } + $dllPath = "windows_core_dll/x64/Release/portmaster-core.dll" + if (!(Test-Path $dllPath)) { + Write-Error "DLL build failed: portmaster-core.dll not found" + exit 1 + } + $dll = Get-Item $dllPath + if ($dll.Length -eq 0) { + Write-Error "DLL appears to be empty" + exit 1 + } + if ($dll.LastWriteTime -lt (Get-Date).AddMinutes(-5)) { + Write-Error "DLL appears to be stale" + exit 1 + } + Write-Output "DLL verification passed: Size=$($dll.Length) bytes, LastWriteTime=$($dll.LastWriteTime)"
41-41: Add newline at end of file.Add a newline character at the end of the file to follow YAML best practices.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: no new line character at the end of file
(new-line-at-end-of-file)
service/firewall/interception/dnsmonitor/module.go (1)
108-109: Fix typos in function commentThe function comment contains several typos:
- "cashe" should be "cache"
- "assoseted" should be "associated"
Apply this diff:
-// saveIPsInCache saves the provided ips in the dns cashe assoseted with the record Domain and CNAMEs. +// saveIPsInCache saves the provided IPs in the DNS cache associated with the record Domain and CNAMEs.service/firewall/interception/dnsmonitor/eventlistener_linux.go (3)
19-21: Consider adding mutex for thread-safe connection management.The
Listenerstruct could benefit from a mutex to ensure thread-safe access to thevarlinkConnfield, especially since it's modified in a goroutine.type Listener struct { + sync.Mutex varlinkConn *varlink.Connection }
111-118: Document empty method implementations.Add comments explaining why these methods are empty implementations. This helps future maintainers understand if this is intentional or needs implementation.
func (l *Listener) flush() error { + // flush() is a no-op for Linux systemd-resolver as events are processed in real-time return nil } func (l *Listener) stop() error { + // stop() is handled by the worker context cancellation and connection cleanup return nil }
122-123: Define capacity constants and optimize allocations.The magic number
5for initial capacity should be defined as a constant. Consider using a sync.Pool for map and slice allocations if this method is called frequently.+const ( + defaultIPsCapacity = 5 + defaultCNAMEsCapacity = 2 +) -cnames := make(map[string]string) -ips := make([]net.IP, 0, 5) +cnames := make(map[string]string, defaultCNAMEsCapacity) +ips := make([]net.IP, 0, defaultIPsCapacity)service/firewall/dns.go (2)
290-305: LGTM! Consider adding documentation for the record creation.The refactored code is well-structured and maintains the existing functionality. Consider adding a brief comment explaining the ResolvedDomain record creation and its purpose.
Add a comment before line 291:
+// Create a new ResolvedDomain record to track DNS resolution results record := resolver.ResolvedDomain{
Line range hint
312-342: Several improvements needed for the SaveIPsInCache function.
The function comment has typos:
- "cashe" should be "cache"
- "assoseted" should be "associated"
Error handling could be improved:
- The database error handling could be more specific
- Consider logging the IP address in error messages for better debugging
The function could benefit from performance optimization when handling multiple IPs
Here's the suggested implementation:
-// SaveIPsInCache saves the provided ips in the dns cashe assoseted with the record Domain and CNAMEs. +// SaveIPsInCache saves the provided IPs in the DNS cache associated with the record Domain and CNAMEs. +// It skips localhost IPs and handles the creation and updating of IPInfo records in the database. +// Parameters: +// - ips: slice of IP addresses to process +// - profileID: ID used for scoping IPInfo records +// - record: ResolvedDomain record containing domain and CNAME information func SaveIPsInCache(ips []net.IP, profileID string, record resolver.ResolvedDomain) { + // Pre-allocate the slice based on the expected number of non-localhost IPs + validIPs := make([]net.IP, 0, len(ips)) + + // First pass: filter out localhost IPs + for _, ip := range ips { + if netutils.GetIPScope(ip) != netutils.HostLocal { + validIPs = append(validIPs, ip) + } + } + // Package IPs and CNAMEs into IPInfo structs. - for _, ip := range ips { - // Never save domain attributions for localhost IPs. - if netutils.GetIPScope(ip) == netutils.HostLocal { - continue - } - + for _, ip := range validIPs { ipString := ip.String() info, err := resolver.GetIPInfo(profileID, ipString) if err != nil { if !errors.Is(err, database.ErrNotFound) { - log.Errorf("nameserver: failed to search for IP info record: %s", err) + log.Errorf("nameserver: failed to search for IP info record %s: %s", ipString, err) } info = &resolver.IPInfo{ IP: ipString, ProfileID: profileID, } } // Add the new record to the resolved domains for this IP and scope. info.AddDomain(record) // Save if the record is new or has been updated. if err := info.Save(); err != nil { - log.Errorf("nameserver: failed to save IP info record: %s", err) + log.Errorf("nameserver: failed to save IP info record %s: %s", ipString, err) } } }The changes improve:
- Documentation clarity and correctness
- Error message context
- Performance by filtering localhost IPs in a separate pass
- Code organization with better comments
Additionally, consider adding unit tests to verify the behavior with:
- Multiple IPs including localhost IPs
- Database error scenarios
- Various domain and CNAME combinations
Would you like me to help create these unit tests?
service/firewall/packet_handler.go (1)
648-655: Consider using constants for resolver info values.The resolver info uses hardcoded string values that should be defined as constants to maintain consistency and make updates easier.
Consider defining these as package constants:
+const ( + DNSMonitorName = "DNSRequestObserver" + DNSMonitorType = resolver.ServerTypeFirewall + DNSMonitorSource = resolver.ServerSourceFirewall +) resolverInfo := &resolver.ResolverInfo{ - Name: "DNSRequestObserver", - Type: resolver.ServerTypeFirewall, - Source: resolver.ServerSourceFirewall, + Name: DNSMonitorName, + Type: DNSMonitorType, + Source: DNSMonitorSource, IP: conn.Entity.IP, Domain: conn.Entity.Domain, IPScope: conn.Entity.IPScope, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.github/workflows/windows-dll.yml(1 hunks)service/firewall/dns.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_linux.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_windows.go(1 hunks)service/firewall/interception/dnsmonitor/module.go(1 hunks)service/firewall/packet_handler.go(6 hunks)service/resolver/resolver.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- service/firewall/interception/dnsmonitor/eventlistener_windows.go
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/windows-dll.yml
[warning] 24-24: wrong indentation: expected 6 but found 4
(indentation)
[error] 41-41: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: Linter
service/firewall/interception/dnsmonitor/eventlistener_linux.go
[failure] 12-12:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 16-16:
File is not gci-ed with --skip-generated -s standard -s default (gci)
service/firewall/interception/dnsmonitor/module.go
[failure] 8-8:
File is not gci-ed with --skip-generated -s standard -s default (gci)
🔇 Additional comments (17)
.github/workflows/windows-dll.yml (3)
3-18: LGTM! Well-structured workflow triggers.
The trigger configuration follows best practices with appropriate path and branch filters.
24-41:
Implement code signing for the Windows DLL.
For security and consistency with other Windows components in the repository, code signing should be implemented.
Add these steps after the build verification:
- name: Setup Code Signing Certificate
run: |
echo "${{ secrets.WINDOWS_PFX_BASE64 }}" | base64 --decode > signing-cert.pfx
shell: bash
- name: Sign DLL
run: |
signtool sign /f signing-cert.pfx /p "${{ secrets.WINDOWS_PFX_PASSWORD }}" /tr http://timestamp.digicert.com /td sha256 /fd sha256 "windows_core_dll/x64/Release/portmaster-core.dll"
- name: Cleanup Certificate
if: always()
run: rm -f signing-cert.pfx
shell: bashNote: This requires setting up WINDOWS_PFX_BASE64 and WINDOWS_PFX_PASSWORD secrets in the repository.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 24-24: wrong indentation: expected 6 but found 4
(indentation)
[error] 41-41: no new line character at the end of file
(new-line-at-end-of-file)
28-29: 🛠️ Refactor suggestion
Enhance build step with error handling and logging.
The build step could benefit from better error handling and build logging.
Apply this diff to enhance the build step:
- name: Build DLL
- run: msbuild windows_core_dll\windows_core_dll.sln -t:rebuild -property:Configuration=Release
+ run: |
+ msbuild windows_core_dll\windows_core_dll.sln `
+ -t:rebuild `
+ -property:Configuration=Release `
+ -verbosity:normal `
+ -consoleloggerparameters:ErrorsOnly `
+ -fileLogger `
+ -fileLoggerParameters:LogFile=msbuild.log;Append;Verbosity=diagnosticLikely invalid or redundant comment.
service/firewall/interception/dnsmonitor/module.go (4)
35-45:
Return error when listener initialization fails
The Start() method should return the error encountered during listener initialization instead of always returning nil. This ensures that any issues during startup are propagated to the caller for proper handling.
Apply this diff:
func (dl *DNSMonitor) Start() error {
// Initialize dns event listener
var err error
dl.listener, err = newListener(dl)
if err != nil {
log.Errorf("dnsmonitor: failed to start dns listener: %s", err)
+ return err
}
- return nil
+ return err
}47-56:
Return error when listener shutdown fails
The Stop() method should return any error encountered when stopping the listener.
Apply this diff:
func (dl *DNSMonitor) Stop() error {
if dl.listener != nil {
err := dl.listener.stop()
if err != nil {
log.Errorf("dnsmonitor: failed to close listener: %s", err)
+ return err
}
}
- return nil
+ return nil
}58-62:
Add nil check in Flush method
The Flush() method should check if the listener exists before calling flush to prevent nil pointer dereference.
Apply this diff:
func (dl *DNSMonitor) Flush() error {
+ if dl.listener == nil {
+ return nil
+ }
return dl.listener.flush()
}63-71:
Set appropriate TTL for domain records
Setting Expires to 0 could cause records to expire immediately. Consider implementing a TTL-based expiration strategy.
Consider this approach:
record := resolver.ResolvedDomain{
Domain: fqdn,
Resolver: &ResolverInfo,
DNSRequestContext: &resolver.DNSRequestContext{},
- Expires: 0,
+ Expires: time.Now().Add(resolver.DefaultTTL).Unix(),
}service/firewall/interception/dnsmonitor/eventlistener_linux.go (2)
35-42: Simplify restart attempts counter.
Move the increment before the condition check for better readability, as suggested in the previous review.
restartAttempts := 0
+restartAttempts++
-if restartAttempts > 10 {
+if restartAttempts > maxRestartAttempts {
return nil
}
-restartAttempts += 144-48: Use module context instead of manager context.
As suggested in the previous review, use module.Ctx() instead of module.mgr.Ctx() for better encapsulation.
-varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor")
+varlinkConn, err := varlink.NewConnection(module.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor")service/resolver/resolver.go (2)
20-27: Verify the integration of new server types
The new server types ServerTypeMonitor and ServerTypeFirewall have been added following consistent naming patterns. Let's verify their usage in the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
New server types are properly integrated and used in the codebase
The new server types are well-integrated and used appropriately:
ServerTypeMonitoris used in the DNS monitor module (service/firewall/interception/dnsmonitor/module.go)ServerTypeFirewallis used in the firewall packet handler (service/firewall/packet_handler.go) along with its corresponding source typeServerSourceFirewall
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the new server types are used in the codebase
# Search for usage of new server types
echo "=== Searching for ServerTypeMonitor usage ==="
rg "ServerTypeMonitor|\"monitor\"" -A 3
echo "=== Searching for ServerTypeFirewall usage ==="
rg "ServerTypeFirewall|\"firewall\"" -A 3
# Search for switch statements handling server types to ensure they're updated
echo "=== Checking switch statements handling server types ==="
ast-grep --pattern 'switch $x.Type {
$$$
}'
Length of output: 3053
33-35: Verify platform-specific implementations
The new server sources (ServerSourceETW, ServerSourceSystemd, ServerSourceFirewall) indicate cross-platform DNS event monitoring. Ensure proper platform-specific implementations exist.
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have sufficient information about the platform-specific implementations:
Platform-specific implementations are properly implemented
The new server sources have appropriate platform-specific implementations:
-
ETW (Windows):
- Implementation in
service/integration/etw_windows.gowith proper build constraints - Windows-specific event listener in
service/firewall/interception/dnsmonitor/eventlistener_windows.go - ETW session management in
service/firewall/interception/dnsmonitor/etwlink_windows.go
- Implementation in
-
Systemd (Linux):
- Linux-specific event listener in
service/firewall/interception/dnsmonitor/eventlistener_linux.go - Systemd resolver monitor integration via varlink protocol
- Proper build constraints with
// +build linux
- Linux-specific event listener in
-
Default implementation for other platforms in
service/firewall/interception/dnsmonitor/eventlistener.gowith// +build !linux,!windows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check platform-specific implementations
# Search for Windows-specific ETW implementation
echo "=== Checking ETW implementation ==="
rg -g "*.go" "ETW|EventTracing" -A 5
# Search for Linux-specific systemd implementation
echo "=== Checking systemd implementation ==="
rg -g "*.go" "systemd|SystemdResolver" -A 5
# Check for build constraints
echo "=== Checking build constraints ==="
rg "^//\s*\+build" -g "*.go"
Length of output: 26951
service/firewall/dns.go (2)
307-311: LGTM! Clear and focused utility function.
The formatRR function is well-implemented and serves its purpose effectively.
304-304: Verify usage of SaveIPsInCache across the codebase
Let's ensure all callers are properly using the new SaveIPsInCache function.
✅ Verification successful
Based on the search results, I can now generate the final response:
SaveIPsInCache is correctly used in the codebase
The function SaveIPsInCache is only called once in service/firewall/dns.go and is properly defined in the same file. The function is also referenced in service/firewall/interception/dnsmonitor/module.go as saveIPsInCache, which follows the same implementation pattern. Both usages are consistent with the DNS caching behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of SaveIPsInCache to verify correct usage
# Search for direct function calls
echo "Direct function calls:"
rg "SaveIPsInCache\(" --type go
# Search for related DNS caching functions to ensure we haven't missed any places
# that should be using the new function
echo -e "\nRelated DNS caching functions:"
rg "func.*(DNS|Cache|IP).*" --type go
Length of output: 25791
service/firewall/packet_handler.go (4)
450-452: LGTM: DNS query interception check is properly guarded.
The addition of the resolver disabled check is correct and prevents DNS query interception when the resolver is disabled.
485-487: Prevent potential duplicate packet inspection.
The code first calls inspectDNSPacket directly and then updates the handler to inspectDNSPacket. This could lead to duplicate inspection of the same packet.
597-601: Improve error handling consistency.
Error handling is inconsistent. Some errors are ignored with _ while others are logged.
616-623:
Consider race condition in deferred packet acceptance.
The packet is accepted in a deferred function, but this happens before we've finished processing the DNS answer and updating the cache. This could lead to a race condition where the packet is accepted before the cache is updated.
Move the packet acceptance after the cache update at line 678 to ensure proper ordering:
- // Packet was parsed.
- // Allow it but only after the answer was added to the cache.
- defer func() {
- err = pkt.Accept()
- if err != nil {
- log.Errorf("filter: failed to accept dns packet: %s", err)
- }
- }()Likely invalid or redundant comment.
| var ResolverInfo = resolver.ResolverInfo{ | ||
| Name: "SystemResolver", | ||
| Type: resolver.ServerTypeMonitor, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using more specific resolver types and sources
As suggested in the previous review, consider using more specific source types to clearly indicate the origin of DNS events. For example:
var ResolverInfo = resolver.ResolverInfo{
Name: "SystemResolver",
- Type: resolver.ServerTypeMonitor,
+ Type: resolver.ServerTypeMonitor,
+ Source: resolver.ServerSourceSystemd, // or "etw" for Windows, "packet" for packet capture
}Committable suggestion skipped: line range outside the PR's diff.
| func processIfSelfCheckDomain(fqdn string) bool { | ||
| // Check for compat check dns request. | ||
| if strings.HasSuffix(fqdn, compat.DNSCheckInternalDomainScope) { | ||
| subdomain := strings.TrimSuffix(fqdn, compat.DNSCheckInternalDomainScope) | ||
| _ = compat.SubmitDNSCheckDomain(subdomain) | ||
| log.Infof("dnsmonitor: self-check domain received") | ||
| // No need to parse the answer. | ||
| return true | ||
| } |
There was a problem hiding this comment.
Handle error from SubmitDNSCheckDomain
The error from compat.SubmitDNSCheckDomain() is being silently discarded. Consider logging or handling the error appropriately.
Apply this diff:
if strings.HasSuffix(fqdn, compat.DNSCheckInternalDomainScope) {
subdomain := strings.TrimSuffix(fqdn, compat.DNSCheckInternalDomainScope)
- _ = compat.SubmitDNSCheckDomain(subdomain)
+ if err := compat.SubmitDNSCheckDomain(subdomain); err != nil {
+ log.Warningf("dnsmonitor: failed to submit DNS check domain: %s", err)
+ }
log.Infof("dnsmonitor: self-check domain received")
return true
}📝 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.
| func processIfSelfCheckDomain(fqdn string) bool { | |
| // Check for compat check dns request. | |
| if strings.HasSuffix(fqdn, compat.DNSCheckInternalDomainScope) { | |
| subdomain := strings.TrimSuffix(fqdn, compat.DNSCheckInternalDomainScope) | |
| _ = compat.SubmitDNSCheckDomain(subdomain) | |
| log.Infof("dnsmonitor: self-check domain received") | |
| // No need to parse the answer. | |
| return true | |
| } | |
| func processIfSelfCheckDomain(fqdn string) bool { | |
| // Check for compat check dns request. | |
| if strings.HasSuffix(fqdn, compat.DNSCheckInternalDomainScope) { | |
| subdomain := strings.TrimSuffix(fqdn, compat.DNSCheckInternalDomainScope) | |
| if err := compat.SubmitDNSCheckDomain(subdomain); err != nil { | |
| log.Warningf("dnsmonitor: failed to submit DNS check domain: %s", err) | |
| } | |
| log.Infof("dnsmonitor: self-check domain received") | |
| // No need to parse the answer. | |
| return true | |
| } |
| _, err := os.Stat("/run/systemd/resolve/io.systemd.Resolve.Monitor") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("system does not support systemd resolver monitor") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for systemd-resolver check.
The current error handling only checks if the error exists but doesn't distinguish between different error types. This could mask other potential issues.
_, err := os.Stat("/run/systemd/resolve/io.systemd.Resolve.Monitor")
if err != nil {
+ if os.IsNotExist(err) {
return nil, fmt.Errorf("system does not support systemd resolver monitor")
+ }
+ return nil, fmt.Errorf("failed to check systemd resolver monitor: %w", err)
}📝 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.
| _, err := os.Stat("/run/systemd/resolve/io.systemd.Resolve.Monitor") | |
| if err != nil { | |
| return nil, fmt.Errorf("system does not support systemd resolver monitor") | |
| } | |
| _, err := os.Stat("/run/systemd/resolve/io.systemd.Resolve.Monitor") | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return nil, fmt.Errorf("system does not support systemd resolver monitor") | |
| } | |
| return nil, fmt.Errorf("failed to check systemd resolver monitor: %w", err) | |
| } |
service/resolver/resolver.go
Outdated
| // Possible values include dns, tcp, dot, doh, mdns, env, packet. | ||
| Type string |
There was a problem hiding this comment.
Resolve inconsistency in Type field documentation
The Type field documentation includes 'packet' as a possible value, but this isn't defined in the ServerType* constants above. This could lead to confusion or runtime issues.
Consider either:
- Adding
ServerTypePacketconstant, or - Removing 'packet' from the documentation if it's not a valid type
- // Possible values include dns, tcp, dot, doh, mdns, env, packet.
+ // Possible values include dns, tcp, dot, doh, mdns, env, monitor, firewall.Committable suggestion skipped: line range outside the PR's diff.
vlabo
force-pushed
the
feature/systemd-query-events
branch
from
831cb33 to
eda62f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
.github/workflows/windows-dll.yml (3)
3-18: Add concurrency protection to prevent simultaneous buildsWhile the trigger configuration is well-structured, concurrent builds could lead to race conditions or resource conflicts.
Add concurrency configuration to cancel in-progress builds:
name: Windows Portmaster Core DLL +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true on: push: paths:
24-27: Fix step indentationThe steps should be indented with 6 spaces to match GitHub Actions YAML conventions.
Apply this diff to fix the indentation:
steps: - - name: Checkout Repository - uses: actions/checkout@v4 - - name: Add msbuild to PATH - uses: microsoft/setup-msbuild@v2 + - name: Checkout Repository + uses: actions/checkout@v4 + - name: Add msbuild to PATH + uses: microsoft/setup-msbuild@v2🧰 Tools
🪛 yamllint (1.35.1)
[warning] 24-24: wrong indentation: expected 6 but found 4
(indentation)
37-41: Add artifact retention and fix file formatThe artifact upload should specify a retention period, and the file should end with a newline.
Apply this diff:
- name: Upload artifacts uses: actions/upload-artifact@v4 with: name: portmaster-core-dll path: windows_core_dll/x64/Release/portmaster-core.dll + retention-days: 30 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: no new line character at the end of file
(new-line-at-end-of-file)
service/firewall/interception/dnsmonitor/eventlistener_linux.go (3)
19-21: Add documentation for the Listener struct.Consider adding a doc comment explaining the purpose and responsibility of the Listener struct.
+// Listener handles DNS query monitoring through systemd-resolver's varlink interface. type Listener struct { varlinkConn *varlink.Connection }
111-118: Document empty interface methods.Add documentation explaining why these interface methods are empty. This helps future maintainers understand the design decision.
+// flush implements the EventListener interface. +// It's a no-op for systemd resolver as events are processed in real-time. func (l *Listener) flush() error { // Nothing to flush return nil } +// stop implements the EventListener interface. +// The connection cleanup is handled by the worker's defer block. func (l *Listener) stop() error { return nil }
120-144: Improve processAnswer implementation.Several minor improvements could be made:
- Define the initial capacity as a constant
- Fix typo in comment
- Consider error handling for saveDomain
+const ( + // initialIPCapacity is the initial capacity for IP slice + initialIPCapacity = 5 +) func (l *Listener) processAnswer(domain string, queryResult *QueryResult) { // Allocated data struct for the parsed result. cnames := make(map[string]string) - ips := make([]net.IP, 0, 5) + ips := make([]net.IP, 0, initialIPCapacity) // Check if the query is valid if queryResult.Answer == nil { return } - // Go trough each answer entry. + // Go through each answer entry. for _, a := range *queryResult.Answer { // ... rest of the function ... } - saveDomain(domain, ips, cnames) + if err := saveDomain(domain, ips, cnames); err != nil { + log.Warningf("dnsmonitor: failed to save domain %s: %v", domain, err) + } }service/firewall/dns.go (2)
290-304: Consider adding error handling for AddCNAMEsThe refactored code looks good and follows a clear structure. However, consider adding error handling for the
AddCNAMEscall to ensure robustness.- record.AddCNAMEs(cnames) + if err := record.AddCNAMEs(cnames); err != nil { + log.Errorf("failed to add CNAMEs: %v", err) + }
Line range hint
312-340: Multiple improvements needed for SaveIPsInCache
- Fix typos in the function comment
- Enhance error handling with more context
- Add debug logging for skipped localhost IPs
- Consider batch processing for better performance
-// SaveIPsInCache saves the provided ips in the dns cashe assoseted with the record Domain and CNAMEs. +// SaveIPsInCache saves the provided IPs in the DNS cache associated with the record Domain and CNAMEs. func SaveIPsInCache(ips []net.IP, profileID string, record resolver.ResolvedDomain) { + // Prepare batch of IPInfo records for bulk processing + var updatedRecords []*resolver.IPInfo + // Package IPs and CNAMEs into IPInfo structs. for _, ip := range ips { // Never save domain attributions for localhost IPs. if netutils.GetIPScope(ip) == netutils.HostLocal { + log.Tracef("skipping localhost IP %s", ip) continue } ipString := ip.String() info, err := resolver.GetIPInfo(profileID, ipString) if err != nil { if !errors.Is(err, database.ErrNotFound) { - log.Errorf("nameserver: failed to search for IP info record: %s", err) + log.Errorf("nameserver: failed to search for IP info record %s (profile: %s): %v", + ipString, profileID, err) } info = &resolver.IPInfo{ IP: ipString, ProfileID: profileID, } } // Add the new record to the resolved domains for this IP and scope. info.AddDomain(record) - // Save if the record is new or has been updated. - if err := info.Save(); err != nil { - log.Errorf("nameserver: failed to save IP info record: %s", err) - } + updatedRecords = append(updatedRecords, info) } + + // Batch save all updated records + for _, info := range updatedRecords { + if err := info.Save(); err != nil { + log.Errorf("nameserver: failed to save IP info record %s (profile: %s): %v", + info.IP, info.ProfileID, err) + } + } }service/firewall/packet_handler.go (2)
450-452: Consider extracting DNS interception conditions for better readability.The condition chain for DNS query interception is quite long. Consider extracting it into a helper function for better readability and maintainability.
+func shouldInterceptDNSQuery(conn *network.Connection, pkt packet.Packet) bool { + resolver := module.instance.Resolver() + if resolver == nil { + return false + } + return dnsQueryInterception() && + !resolver.IsDisabled() && + pkt.IsOutbound() && + pkt.Info().DstPort == 53 && + nameserverIPMatcherReady.IsSet() && + !nameserverIPMatcher(pkt.Info().Dst) && + !(conn.Process().Pid == ownPID && + conn.Entity.IPScope == netutils.LocalMulticast) +} -case dnsQueryInterception() && - !module.instance.Resolver().IsDisabled() && +case shouldInterceptDNSQuery(conn, pkt):
648-655: Consider extracting resolver info constants.The hardcoded values for resolver info could be moved to package-level constants for better maintainability.
+const ( + DNSMonitorName = "DNSRequestObserver" + DNSMonitorType = resolver.ServerTypeFirewall + DNSMonitorSource = resolver.ServerSourceFirewall +) resolverInfo := &resolver.ResolverInfo{ - Name: "DNSRequestObserver", - Type: resolver.ServerTypeFirewall, - Source: resolver.ServerSourceFirewall, + Name: DNSMonitorName, + Type: DNSMonitorType, + Source: DNSMonitorSource, IP: conn.Entity.IP, Domain: conn.Entity.Domain, IPScope: conn.Entity.IPScope, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.github/workflows/windows-dll.yml(1 hunks)service/firewall/dns.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_linux.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_windows.go(1 hunks)service/firewall/interception/dnsmonitor/module.go(1 hunks)service/firewall/packet_handler.go(6 hunks)service/resolver/resolver.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- service/firewall/interception/dnsmonitor/eventlistener_windows.go
- service/resolver/resolver.go
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/windows-dll.yml
[warning] 24-24: wrong indentation: expected 6 but found 4
(indentation)
[error] 41-41: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: Linter
service/firewall/interception/dnsmonitor/eventlistener_linux.go
[failure] 12-12:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 16-16:
File is not gci-ed with --skip-generated -s standard -s default (gci)
service/firewall/interception/dnsmonitor/module.go
[failure] 8-8:
File is not gci-ed with --skip-generated -s standard -s default (gci)
🔇 Additional comments (15)
.github/workflows/windows-dll.yml (2)
19-27:
Add code signing setup
The DLL should be signed for security, as evidenced by code signing being used elsewhere in the repository.
Add these steps before the build:
steps:
- name: Checkout Repository
uses: actions/checkout@v4
- name: Add msbuild to PATH
uses: microsoft/setup-msbuild@v2
+ - name: Install code signing certificate
+ run: |
+ echo "${{ secrets.WINDOWS_CERT_BASE64 }}" | base64 --decode > code_signing.pfx
+ - name: Add code signing tool to PATH
+ uses: microsoft/setup-signtool@v1🧰 Tools
🪛 yamllint (1.35.1)
[warning] 24-24: wrong indentation: expected 6 but found 4
(indentation)
28-36: 🛠️ Refactor suggestion
Enhance build configuration and verification
The build step could benefit from additional security and optimization parameters.
Apply this diff to enhance the build and verification:
- name: Build DLL
- run: msbuild windows_core_dll\windows_core_dll.sln -t:rebuild -property:Configuration=Release
+ run: |
+ msbuild windows_core_dll\windows_core_dll.sln -t:rebuild `
+ -property:Configuration=Release `
+ -property:Platform=x64 `
+ -property:EnableNETAnalyzers=true `
+ -property:AnalysisMode=Recommended `
+ -property:TreatWarningsAsErrors=true
- name: Verify DLL
shell: powershell
run: |
+ # Verify file exists
if (!(Test-Path "windows_core_dll/x64/Release/portmaster-core.dll")) {
Write-Error "DLL build failed: portmaster-core.dll not found"
exit 1
}
+ # Verify file signature and attributes
+ $dll = Get-Item "windows_core_dll/x64/Release/portmaster-core.dll"
+ if ($dll.Length -eq 0) {
+ Write-Error "DLL is empty"
+ exit 1
+ }Likely invalid or redundant comment.
service/firewall/interception/dnsmonitor/module.go (7)
18-21: Enhance resolver source identification
Following the previous review suggestion, consider using more specific source types to clearly indicate the origin of DNS events (e.g., "systemd", "etw", or "packet").
var ResolverInfo = resolver.ResolverInfo{
Name: "SystemResolver",
Type: resolver.ServerTypeMonitor,
+ Source: resolver.ServerSourceSystemd, // Use appropriate source based on the platform
}36-45: Return error when listener initialization fails
The Start() method should return the error encountered during listener initialization instead of only logging it.
func (dl *DNSMonitor) Start() error {
// Initialize dns event listener
var err error
dl.listener, err = newListener(dl)
if err != nil {
log.Errorf("dnsmonitor: failed to start dns listener: %s", err)
+ return err
}
- return nil
+ return err
}48-56: Return error when listener shutdown fails
The Stop() method should return any error encountered when stopping the listener.
func (dl *DNSMonitor) Stop() error {
if dl.listener != nil {
err := dl.listener.stop()
if err != nil {
log.Errorf("dnsmonitor: failed to close listener: %s", err)
+ return err
}
}
- return nil
+ return nil
}59-61: Add nil check in Flush method
The Flush() method should check if the listener is initialized before calling flush.
func (dl *DNSMonitor) Flush() error {
+ if dl.listener == nil {
+ return nil
+ }
return dl.listener.flush()
}66-71: Set appropriate TTL for resolved domains
The Expires field is set to 0, which could cause records to expire immediately.
Consider setting an appropriate TTL value based on the DNS response or a default caching duration.
95-103: Handle error from SubmitDNSCheckDomain
The error from compat.SubmitDNSCheckDomain() should not be silently discarded.
if strings.HasSuffix(fqdn, compat.DNSCheckInternalDomainScope) {
subdomain := strings.TrimSuffix(fqdn, compat.DNSCheckInternalDomainScope)
- _ = compat.SubmitDNSCheckDomain(subdomain)
+ if err := compat.SubmitDNSCheckDomain(subdomain); err != nil {
+ log.Warningf("dnsmonitor: failed to submit DNS check domain: %s", err)
+ }
log.Infof("dnsmonitor: self-check domain received")
return true
}108-138: LGTM! Well-structured IP caching implementation
The implementation correctly:
- Skips localhost IPs
- Handles database errors appropriately
- Uses proper error logging
service/firewall/dns.go (2)
307-310: LGTM!
The formatRR function is well-documented and correctly implements the formatting of DNS resource records.
Line range hint 290-340: Verify protection against DNS-related security issues
Please ensure the implementation has adequate protection against:
- DNS cache poisoning: Verify that the DNS response validation is properly handled upstream
- Resource exhaustion: Consider implementing limits on:
- Maximum number of CNAMEs per domain
- Maximum cache size per profile
- Maximum number of resolved domains per IP
✅ Verification successful
Based on the gathered information, I can now generate the final response:
DNS security measures are adequately implemented
The codebase shows several security measures in place:
- CNAME chain depth is limited to 50 levels to prevent infinite loops and resource exhaustion
- Cache size is limited with configurable values (default 256 entries)
- DNS response validation includes:
- Proper handling of resolver failures and timeouts
- TLS support for secure DNS protocols (DoT, DoH)
- Validation of DNS message IDs to prevent spoofing
- Local IP addresses are explicitly filtered from caching to prevent DNS rebinding attacks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for DNS validation and resource limits in the codebase
# Look for DNS response validation
echo "Checking for DNS response validation..."
rg -A 5 "func.*validate.*DNS"
# Look for cache size limits
echo "Checking for cache size limits..."
rg -A 5 "MaxCache|CacheSize|LimitCache"
# Look for CNAME chain limits
echo "Checking for CNAME chain limits..."
rg -A 5 "MaxCNAME|CNAMELimit|LimitCNAME"
Length of output: 4706
Script:
#!/bin/bash
# Let's check for DNS validation and security measures in a different way
# Look for DNS response validation in the resolver package
echo "Checking DNS validation in resolver..."
rg -A 10 "func.*Resolve" service/resolver/
# Check for CNAME handling and limits
echo "Checking CNAME handling..."
rg -A 10 "AddCNAMEs|CNAMEs" service/resolver/
# Check for DNS security measures
echo "Checking DNS security measures..."
rg -A 10 "security|validate|verify" service/firewall/dns.go
# Check for cache implementation details
echo "Checking cache implementation..."
ast-grep --pattern 'type ResolvedDomain struct {
$$$
}'
Length of output: 51324
service/firewall/packet_handler.go (4)
485-487: Prevent potential duplicate packet inspection.
The code first calls inspectDNSPacket directly and then updates the handler to inspectDNSPacket. This could lead to duplicate inspection of the same packet.
596-601: Improve error handling consistency.
Error handling is inconsistent. Some errors are ignored with _ while others are logged.
626-632: Add bounds check for DNS question array.
The code assumes that the question array has at least one element without checking.
589-679: 🛠️ Refactor suggestion
Consider splitting the function into smaller, focused functions.
The inspectDNSPacket function is quite long and handles multiple responsibilities:
- Packet validation
- DNS message parsing
- Compat check handling
- Cache updating
Consider splitting it into smaller, focused functions to improve maintainability and testability.
+func validateDNSPacket(pkt packet.Packet) (*dns.Msg, error) {
+ dnsPacket := new(dns.Msg)
+ if err := pkt.LoadPacketData(); err != nil {
+ return nil, fmt.Errorf("failed to load packet data: %w", err)
+ }
+ if err := dnsPacket.Unpack(pkt.Payload()); err != nil {
+ return nil, fmt.Errorf("failed to unpack DNS message: %w", err)
+ }
+ return dnsPacket, nil
+}
+
+func handleCompatCheck(fqdn string) bool {
+ if strings.HasSuffix(fqdn, compat.DNSCheckInternalDomainScope) {
+ subdomain := strings.TrimSuffix(fqdn, compat.DNSCheckInternalDomainScope)
+ _ = compat.SubmitDNSCheckDomain(subdomain)
+ log.Infof("packet_handler: self-check domain received")
+ return true
+ }
+ return false
+}
+
+func createResolverCache(query *dns.Question, dnsPacket *dns.Msg, conn *network.Connection) *resolver.RRCache {
+ resolverInfo := &resolver.ResolverInfo{
+ Name: DNSMonitorName,
+ Type: DNSMonitorType,
+ Source: DNSMonitorSource,
+ IP: conn.Entity.IP,
+ Domain: conn.Entity.Domain,
+ IPScope: conn.Entity.IPScope,
+ }
+
+ return &resolver.RRCache{
+ Domain: dns.Fqdn(query.Name),
+ Question: dns.Type(query.Qtype),
+ RCode: dnsPacket.Rcode,
+ Answer: dnsPacket.Answer,
+ Ns: dnsPacket.Ns,
+ Extra: dnsPacket.Extra,
+ Resolver: resolverInfo,
+ }
+}
+
func inspectDNSPacket(conn *network.Connection, pkt packet.Packet) {
if pkt.InfoOnly() {
return
}
- dnsPacket := new(dns.Msg)
- err := pkt.LoadPacketData()
+ dnsPacket, err := validateDNSPacket(pkt)
if err != nil {
- _ = pkt.Block()
- log.Errorf("filter: failed to load packet payload: %s", err)
+ if blockErr := pkt.Block(); blockErr != nil {
+ log.Errorf("filter: failed to block packet: %s", blockErr)
+ }
+ log.Errorf("filter: %s", err)
return
}
- // Parse and block invalid packets.
- err = dnsPacket.Unpack(pkt.Payload())
- if err != nil {
- err = pkt.PermanentBlock()
- if err != nil {
- log.Errorf("filter: failed to block packet: %s", err)
- }
- _ = conn.SetVerdict(network.VerdictBlock, "none DNS data on DNS port", "", nil)
- conn.VerdictPermanent = true
- conn.Save()
+ if len(dnsPacket.Question) == 0 {
return
}
+ question := dnsPacket.Question[0]
+ fqdn := dns.Fqdn(question.Name)
+
+ if handleCompatCheck(fqdn) {
+ return
+ }
+
+ if len(dnsPacket.Answer) == 0 {
+ return
+ }
+
+ rrCache := createResolverCache(&question, dnsPacket, conn)
+ query := &resolver.Query{
+ FQDN: fqdn,
+ QType: dns.Type(question.Qtype),
+ NoCaching: false,
+ IgnoreFailing: false,
+ LocalResolversOnly: false,
+ ICANNSpace: false,
+ DomainRoot: "",
+ }
+
+ UpdateIPsAndCNAMEs(query, rrCache, conn)
+}Likely invalid or redundant comment.
| module.mgr.Go("systemd-resolver-event-listener", func(w *mgr.WorkerCtx) error { | ||
| // Abort initialization if the connection failed after too many tries. | ||
| if restartAttempts > 10 { | ||
| return nil | ||
| } | ||
| restartAttempts += 1 | ||
|
|
||
| // Initialize varlink connection | ||
| varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to connect to systemd-resolver varlink service: %w", err) | ||
| } | ||
| defer func() { | ||
| if varlinkConn != nil { | ||
| err = varlinkConn.Close() | ||
| if err != nil { | ||
| log.Errorf("dnsmonitor: failed to close varlink connection: %s", err) | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| listener.varlinkConn = varlinkConn | ||
| // Subscribe to the dns query events | ||
| receive, err := listener.varlinkConn.Send(w.Ctx(), "io.systemd.Resolve.Monitor.SubscribeQueryResults", nil, varlink.More) | ||
| if err != nil { | ||
| var varlinkErr *varlink.Error | ||
| if errors.As(err, &varlinkErr) { | ||
| return fmt.Errorf("failed to issue Varlink call: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to issue Varlink call: %w", err) | ||
| } | ||
| } | ||
|
|
||
| for { | ||
| queryResult := QueryResult{} | ||
| // Receive the next event from the resolver. | ||
| flags, err := receive(w.Ctx(), &queryResult) | ||
| if err != nil { | ||
| var varlinkErr *varlink.Error | ||
| if errors.As(err, &varlinkErr) { | ||
| return fmt.Errorf("failed to receive Varlink reply: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to receive Varlink reply: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // Check if the reply indicates the end of the stream | ||
| if flags&varlink.Continues == 0 { | ||
| break | ||
| } | ||
|
|
||
| // Ignore if there is no question. | ||
| if queryResult.Question == nil || len(*queryResult.Question) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| // Protmaster self check | ||
| domain := (*queryResult.Question)[0].Name | ||
| if processIfSelfCheckDomain(dns.Fqdn(domain)) { | ||
| // Not need to process result. | ||
| continue | ||
| } | ||
|
|
||
| if queryResult.Rcode != nil { | ||
| continue // Ignore DNS errors | ||
| } | ||
|
|
||
| listener.processAnswer(domain, &queryResult) | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve worker error handling and connection management.
The worker function could benefit from better error handling and connection management:
- The connection cleanup in the defer block could race with the listener.varlinkConn assignment
- Error handling for varlink calls is repetitive
Consider refactoring like this:
+ // Move connection to a local var first to avoid races
+ localConn := varlinkConn
+ listener.varlinkConn = localConn
+
+ // Helper for varlink error handling
+ handleVarlinkError := func(err error, op string) error {
+ var varlinkErr *varlink.Error
+ if errors.As(err, &varlinkErr) {
+ return fmt.Errorf("failed to %s: %+v", op, varlinkErr.Parameters)
+ }
+ return fmt.Errorf("failed to %s: %w", op, err)
+ }
+
// Subscribe to the dns query events
- receive, err := listener.varlinkConn.Send(w.Ctx(), "io.systemd.Resolve.Monitor.SubscribeQueryResults", nil, varlink.More)
+ receive, err := localConn.Send(w.Ctx(), "io.systemd.Resolve.Monitor.SubscribeQueryResults", nil, varlink.More)
if err != nil {
- var varlinkErr *varlink.Error
- if errors.As(err, &varlinkErr) {
- return fmt.Errorf("failed to issue Varlink call: %+v", varlinkErr.Parameters)
- } else {
- return fmt.Errorf("failed to issue Varlink call: %w", err)
- }
+ return handleVarlinkError(err, "issue Varlink call")
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
ResolvedDomainstruct.Compatmodule with a new resolver integration method.Instancestruct for OS integration and DNS monitoring.Bug Fixes
Refactor
Documentation