Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/systemd query events #1728

Merged
merged 15 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ require (
github.com/hashicorp/go-version v1.7.0
github.com/jackc/puddle/v2 v2.2.1
github.com/lmittmann/tint v1.0.5
github.com/maruel/panicparse/v2 v2.3.1
github.com/mat/besticon v3.12.0+incompatible
github.com/mattn/go-colorable v0.1.13
github.com/mattn/go-isatty v0.0.20
Expand All @@ -57,6 +58,7 @@ require (
github.com/tidwall/gjson v1.17.3
github.com/tidwall/sjson v1.2.5
github.com/umahmood/haversine v0.0.0-20151105152445-808ab04add26
github.com/varlink/go v0.4.0
github.com/vincent-petithory/dataurl v1.0.0
go.etcd.io/bbolt v1.3.10
golang.org/x/exp v0.0.0-20240808152545-0cdaa3abc0fa
Expand Down Expand Up @@ -90,7 +92,6 @@ require (
github.com/inconshreveable/mousetrap v1.1.0 // indirect
github.com/josharian/native v1.1.0 // indirect
github.com/klauspost/cpuid/v2 v2.2.8 // indirect
github.com/maruel/panicparse/v2 v2.3.1 // indirect
github.com/mdlayher/netlink v1.7.2 // indirect
github.com/mdlayher/socket v0.5.1 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,8 @@ github.com/valyala/fastrand v1.1.0 h1:f+5HkLW4rsgzdNoleUOB69hyT9IlD2ZQh9GyDMfb5G
github.com/valyala/fastrand v1.1.0/go.mod h1:HWqCzkrkg6QXT8V2EXWvXCoow7vLwOFN002oeRzjapQ=
github.com/valyala/histogram v1.2.0 h1:wyYGAZZt3CpwUiIb9AU/Zbllg1llXyrtApRS815OLoQ=
github.com/valyala/histogram v1.2.0/go.mod h1:Hb4kBwb4UxsaNbbbh+RRz8ZR6pdodR57tzWUS3BUzXY=
github.com/varlink/go v0.4.0 h1:+/BQoUO9eJK/+MTSHwFcJch7TMsb6N6Dqp6g0qaXXRo=
github.com/varlink/go v0.4.0/go.mod h1:DKg9Y2ctoNkesREGAEak58l+jOC6JU2aqZvUYs5DynU=
github.com/vincent-petithory/dataurl v1.0.0 h1:cXw+kPto8NLuJtlMsI152irrVw9fRDX8AbShPRpg2CI=
github.com/vincent-petithory/dataurl v1.0.0/go.mod h1:FHafX5vmDzyP+1CQATJn7WFKc9CvnvxyvZy6I1MrG/U=
github.com/vmihailenco/msgpack/v5 v5.3.5/go.mod h1:7xyJ9e+0+9SaZT0Wt1RGleJXzli6Q/V5KbhBonMG9jc=
Expand Down
9 changes: 9 additions & 0 deletions service/compat/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ func stop() error {
}

func selfcheckTaskFunc(wc *mgr.WorkerCtx) error {
res := module.instance.Resolver()
if res.IsDisabled.IsSet() {
log.Debugf("compat: skipping self-check: resolver is disabled")
return nil
}

// Create tracing logger.
ctx, tracer := log.AddTracer(wc.Ctx())
defer tracer.Submit()
Expand All @@ -118,6 +124,8 @@ func selfcheckTaskFunc(wc *mgr.WorkerCtx) error {
tracer.Warningf("compat: %s", err)
case selfcheckNetworkChangedFlag.IsSet():
// The network changed, ignore the issue.
case res.IsDisabled.IsSet():
// Portmaster resolver is disabled, ignore this issue.
default:
// The self-check failed.

Expand Down Expand Up @@ -181,4 +189,5 @@ func New(instance instance) (*Compat, error) {

type instance interface {
NetEnv() *netenv.NetEnv
Resolver() *resolver.ResolverModule
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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 missing Resolver() method to match the interface requirements
  • Other implementations in service/instance.go and 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:

  1. Find the interface definition to confirm the methods we need to check
  2. 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

}
12 changes: 12 additions & 0 deletions service/firewall/bypassing.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@ func PreventBypassing(ctx context.Context, conn *network.Connection) (endpoints.
return endpoints.NoMatch, "", nil
}

// If Portmaster resolver is disabled allow requests going to system dns resolver.
// And allow all connections out of the System Resolver.
if module.instance.Resolver().IsDisabled.IsSet() {
// TODO(vladimir): Is there a more specific check that can be done?
if conn.Process().IsSystemResolver() {
return endpoints.NoMatch, "", nil
}
if conn.Entity.Port == 53 && conn.Entity.IPScope.IsLocalhost() {
return endpoints.NoMatch, "", nil
}
Comment on lines +53 to +55
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

// Block bypass attempts using an (encrypted) DNS server.
switch {
case conn.Entity.Port == 53:
Expand Down
171 changes: 171 additions & 0 deletions service/firewall/interception/dnslistener/module.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
package dnslistener

import (
"errors"
"fmt"
"net"
"sync/atomic"

"github.com/miekg/dns"

Check failure on line 9 in service/firewall/interception/dnslistener/module.go

View workflow job for this annotation

GitHub Actions / Linter

File is not `gci`-ed with --skip-generated -s standard -s default (gci)
"github.com/safing/portmaster/base/log"
"github.com/safing/portmaster/service/mgr"
"github.com/safing/portmaster/service/network/netutils"
"github.com/safing/portmaster/service/resolver"
"github.com/varlink/go/varlink"

Check failure on line 14 in service/firewall/interception/dnslistener/module.go

View workflow job for this annotation

GitHub Actions / Linter

File is not `gci`-ed with --skip-generated -s standard -s default (gci)
)

var ResolverInfo = resolver.ResolverInfo{
Name: "SystemdResolver",
Type: "env",
Source: "System",
}
vlabo marked this conversation as resolved.
Show resolved Hide resolved

type DNSListener struct {
instance instance
mgr *mgr.Manager

varlinkConn *varlink.Connection
}

func (dl *DNSListener) Manager() *mgr.Manager {
return dl.mgr
}

func (dl *DNSListener) Start() error {
var err error

// Create the varlink connection with the systemd resolver.
dl.varlinkConn, err = varlink.NewConnection(dl.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor")
if err != nil {
log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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

}

dl.mgr.Go("systemd-resolver-event-listener", func(w *mgr.WorkerCtx) error {
// Subscribe to the dns query events
receive, err := dl.varlinkConn.Send(dl.mgr.Ctx(), "io.systemd.Resolve.Monitor.SubscribeQueryResults", nil, varlink.More)
if err != nil {
if varlinkErr, ok := err.(*varlink.Error); ok {

Check failure on line 48 in service/firewall/interception/dnslistener/module.go

View workflow job for this annotation

GitHub Actions / Linter

type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
return fmt.Errorf("failed to issue Varlink call: %+v", varlinkErr.Parameters)
} else {
return fmt.Errorf("failed to issue Varlink call: %v", err)

Check failure on line 51 in service/firewall/interception/dnslistener/module.go

View workflow job for this annotation

GitHub Actions / Linter

non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
vlabo marked this conversation as resolved.
Show resolved Hide resolved
vlabo marked this conversation as resolved.
Show resolved Hide resolved
}
}

for {
queryResult := QueryResult{}
// Receive the next event from the resolver.
flags, err := receive(w.Ctx(), &queryResult)
if err != nil {
if varlinkErr, ok := err.(*varlink.Error); ok {

Check failure on line 60 in service/firewall/interception/dnslistener/module.go

View workflow job for this annotation

GitHub Actions / Linter

type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
return fmt.Errorf("failed to receive Varlink reply: %+v", varlinkErr.Parameters)
} else {
return fmt.Errorf("failed to receive Varlink reply: %v", err)

Check failure on line 63 in service/firewall/interception/dnslistener/module.go

View workflow job for this annotation

GitHub Actions / Linter

non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
vlabo marked this conversation as resolved.
Show resolved Hide resolved
vlabo marked this conversation as resolved.
Show resolved Hide resolved
}
}

// Check if the reply indicates the end of the stream
if flags&varlink.Continues == 0 {
break
}

if queryResult.Rcode != nil {
continue // Ignore DNS errors
}

dl.processAnswer(&queryResult)

}

return nil
})

return nil
}

func (dl *DNSListener) processAnswer(queryResult *QueryResult) {
// Allocated data struct for the parsed result.
cnames := make(map[string]string)
ips := make([]net.IP, 0, 5)

// Check if the query is valid
if queryResult.Question == nil || len(*queryResult.Question) == 0 || queryResult.Answer == nil {
return
}

domain := (*queryResult.Question)[0].Name

// Go trough each answer entry.
for _, a := range *queryResult.Answer {
if a.RR.Address != nil {
ip := net.IP(*a.RR.Address)
// Answer contains ip address.
ips = append(ips, ip)

} else if a.RR.Name != nil {
// Answer is a CNAME.
cnames[domain] = *a.RR.Name
}
}

for _, ip := range ips {
// Never save domain attributions for localhost IPs.
if netutils.GetIPScope(ip) == netutils.HostLocal {
continue
}
fqdn := dns.Fqdn(domain)

// Create new record for this IP.
record := resolver.ResolvedDomain{
Domain: fqdn,
Resolver: &ResolverInfo,
DNSRequestContext: &resolver.DNSRequestContext{},
Expires: 0,
}

for {
nextDomain, isCNAME := cnames[domain]
if !isCNAME {
break
}
vlabo marked this conversation as resolved.
Show resolved Hide resolved

record.CNAMEs = append(record.CNAMEs, nextDomain)
domain = nextDomain
}

info := resolver.IPInfo{
IP: ip.String(),
}

// 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)
}
}
}

func (dl *DNSListener) Stop() error {
if dl.varlinkConn != nil {
_ = dl.varlinkConn.Close()
}
return nil
}

var shimLoaded atomic.Bool

func New(instance instance) (*DNSListener, error) {
if !shimLoaded.CompareAndSwap(false, true) {
return nil, errors.New("only one instance allowed")
}
m := mgr.New("DNSListener")
module := &DNSListener{
mgr: m,
instance: instance,
}
return module, nil
}

type instance interface{}
79 changes: 79 additions & 0 deletions service/firewall/interception/dnslistener/varlinktypes.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package dnslistener

// List of struct that define the systemd-resolver varlink dns event protocol.

type ResourceKey struct {
Class int `json:"class"`
Type int `json:"type"`
Name string `json:"name"`
}

type ResourceRecord struct {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Key ResourceKey `json:"key"`
Name *string `json:"name,omitempty"`
Address *[]byte `json:"address,omitempty"`
// Rest of the fields are not used.
// Priority *int `json:"priority,omitempty"`
// Weight *int `json:"weight,omitempty"`
// Port *int `json:"port,omitempty"`
// CPU *string `json:"cpu,omitempty"`
// OS *string `json:"os,omitempty"`
// Items *[]string `json:"items,omitempty"`
// MName *string `json:"mname,omitempty"`
// RName *string `json:"rname,omitempty"`
// Serial *int `json:"serial,omitempty"`
// Refresh *int `json:"refresh,omitempty"`
// Expire *int `json:"expire,omitempty"`
// Minimum *int `json:"minimum,omitempty"`
// Exchange *string `json:"exchange,omitempty"`
// Version *int `json:"version,omitempty"`
// Size *int `json:"size,omitempty"`
// HorizPre *int `json:"horiz_pre,omitempty"`
// VertPre *int `json:"vert_pre,omitempty"`
// Latitude *int `json:"latitude,omitempty"`
// Longitude *int `json:"longitude,omitempty"`
// Altitude *int `json:"altitude,omitempty"`
// KeyTag *int `json:"key_tag,omitempty"`
// Algorithm *int `json:"algorithm,omitempty"`
// DigestType *int `json:"digest_type,omitempty"`
// Digest *string `json:"digest,omitempty"`
// FPType *int `json:"fptype,omitempty"`
// Fingerprint *string `json:"fingerprint,omitempty"`
// Flags *int `json:"flags,omitempty"`
// Protocol *int `json:"protocol,omitempty"`
// DNSKey *string `json:"dnskey,omitempty"`
// Signer *string `json:"signer,omitempty"`
// TypeCovered *int `json:"type_covered,omitempty"`
// Labels *int `json:"labels,omitempty"`
// OriginalTTL *int `json:"original_ttl,omitempty"`
// Expiration *int `json:"expiration,omitempty"`
// Inception *int `json:"inception,omitempty"`
// Signature *string `json:"signature,omitempty"`
// NextDomain *string `json:"next_domain,omitempty"`
// Types *[]int `json:"types,omitempty"`
// Iterations *int `json:"iterations,omitempty"`
// Salt *string `json:"salt,omitempty"`
// Hash *string `json:"hash,omitempty"`
// CertUsage *int `json:"cert_usage,omitempty"`
// Selector *int `json:"selector,omitempty"`
// MatchingType *int `json:"matching_type,omitempty"`
// Data *string `json:"data,omitempty"`
// Tag *string `json:"tag,omitempty"`
// Value *string `json:"value,omitempty"`
}
vlabo marked this conversation as resolved.
Show resolved Hide resolved

type Answer struct {
RR *ResourceRecord `json:"rr,omitempty"`
Raw string `json:"raw"`
IfIndex *int `json:"ifindex,omitempty"`
}

type QueryResult struct {
Ready *bool `json:"ready,omitempty"`
State *string `json:"state,omitempty"`
Rcode *int `json:"rcode,omitempty"`
Errno *int `json:"errno,omitempty"`
Question *[]ResourceKey `json:"question,omitempty"`
CollectedQuestions *[]ResourceKey `json:"collectedQuestions,omitempty"`
Answer *[]Answer `json:"answer,omitempty"`
}
5 changes: 3 additions & 2 deletions service/firewall/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/safing/portmaster/service/netquery"
"github.com/safing/portmaster/service/network"
"github.com/safing/portmaster/service/profile"
"github.com/safing/portmaster/service/resolver"
"github.com/safing/portmaster/spn/access"
"github.com/safing/portmaster/spn/captain"
)
Expand All @@ -34,8 +35,7 @@ func (ss *stringSliceFlag) Set(value string) error {
var allowedClients stringSliceFlag

type Firewall struct {
mgr *mgr.Manager

mgr *mgr.Manager
instance instance
}

Expand Down Expand Up @@ -165,4 +165,5 @@ type instance interface {
Access() *access.Access
Network() *network.Network
NetQuery() *netquery.NetQuery
Resolver() *resolver.ResolverModule
}
3 changes: 2 additions & 1 deletion service/firewall/packet_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -444,8 +444,9 @@ func filterHandler(conn *network.Connection, pkt packet.Packet) {
filterConnection = false
log.Tracer(pkt.Ctx()).Infof("filter: granting own pre-authenticated connection %s", conn)

// Redirect outbound DNS packets if enabled,
// Redirect outbound DNS packets if enabled,
case dnsQueryInterception() &&
module.instance.Resolver().IsDisabled.IsNotSet() &&
pkt.IsOutbound() &&
pkt.Info().DstPort == 53 &&
// that don't match the address of our nameserver,
Expand Down
Loading
Loading