refactor: split allow/deny lists to also preceeding ones

Author: dobrac

packages/api/internal/handlers/sandbox_create.go

@@ -23,7 +23,6 @@ import (
 	"github.com/e2b-dev/infra/packages/db/types"
 	"github.com/e2b-dev/infra/packages/shared/pkg/id"
 	sbxlogger "github.com/e2b-dev/infra/packages/shared/pkg/logger/sandbox"
-	sandbox_network "github.com/e2b-dev/infra/packages/shared/pkg/sandbox-network"
 	"github.com/e2b-dev/infra/packages/shared/pkg/telemetry"
 	sharedUtils "github.com/e2b-dev/infra/packages/shared/pkg/utils"
 )
@@ -296,19 +295,6 @@ func validateNetworkConfig(network *api.SandboxNetworkConfig) *api.APIError {
 		return nil
 	}
 
-	if network.AllowOut != nil {
-		for _, address := range *network.AllowOut {
-			cidr := sandbox_network.AddressStringToCIDR(address)
-			if err := sandbox_network.CanAllowCIDR(cidr); err != nil {
-				return &api.APIError{
-					Code:      http.StatusBadRequest,
-					Err:       fmt.Errorf("invalid allowOut CIDR (%s): %w", cidr, err),
-					ClientMsg: fmt.Sprintf("address is not allowed for allowOut: %s", address),
-				}
-			}
-		}
-	}
-
 	if maskRequestHost := network.MaskRequestHost; maskRequestHost != nil {
 		hostname, _, err := splitHostPortOptional(*maskRequestHost)
 		if err != nil {

packages/api/internal/orchestrator/create_instance.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"slices"
 	"time"
 
 	"go.opentelemetry.io/otel/attribute"
@@ -30,8 +29,7 @@ import (
 )
 
 // buildNetworkConfig constructs the orchestrator network configuration from the input parameters
-func buildNetworkConfig(network *types.SandboxNetworkConfig, allowInternetAccess *bool, trafficAccessToken *string) (n *orchestrator.SandboxNetworkConfig, aia *bool) {
-	aia = allowInternetAccess
+func buildNetworkConfig(network *types.SandboxNetworkConfig, allowInternetAccess *bool, trafficAccessToken *string) *orchestrator.SandboxNetworkConfig {
 	orchNetwork := &orchestrator.SandboxNetworkConfig{
 		Egress: &orchestrator.SandboxNetworkEgressConfig{},
 		Ingress: &orchestrator.SandboxNetworkIngressConfig{
@@ -56,15 +54,7 @@ func buildNetworkConfig(network *types.SandboxNetworkConfig, allowInternetAccess
 		orchNetwork.Egress.DeniedCidrs = []string{sandbox_network.AllInternetTrafficCIDR}
 	}
 
-	if slices.Contains(orchNetwork.GetEgress().GetAllowedCidrs(), sandbox_network.AllInternetTrafficCIDR) {
-		// If all traffic is allowed, clear the allowed and denied lists
-		// Internet access is enabled by default.
-		orchNetwork.Egress.AllowedCidrs = nil
-		orchNetwork.Egress.DeniedCidrs = nil
-		aia = ut.ToPtr(true)
-	}
-
-	return orchNetwork, aia
+	return orchNetwork
 }
 
 func (o *Orchestrator) CreateSandbox(
@@ -194,7 +184,7 @@ func (o *Orchestrator) CreateSandbox(
 		trafficAccessToken = &accessToken
 	}
 
-	sbxNetwork, allowInternetAccess := buildNetworkConfig(network, allowInternetAccess, trafficAccessToken)
+	sbxNetwork := buildNetworkConfig(network, allowInternetAccess, trafficAccessToken)
 	sbxRequest := &orchestrator.SandboxCreateRequest{
 		Sandbox: &orchestrator.SandboxConfig{
 			BaseTemplateId:      baseTemplateID,

packages/orchestrator/internal/sandbox/network/firewall.go

@@ -22,10 +22,13 @@ type Firewall struct {
 	table *nftables.Table
 	chain *nftables.Chain
 
-	blockInternetTraffic bool
-	denySet              set.Set
-	allowSet             set.Set
-	tapInterface         string
+	predefinedDenySet  set.Set
+	predefinedAllowSet set.Set
+
+	userDenySet  set.Set
+	userAllowSet set.Set
+
+	tapInterface string
 
 	allowedRanges []string
 }
@@ -51,6 +54,15 @@ func NewFirewall(tapIf string, hyperloopIP string) (*Firewall, error) {
 	})
 
 	// Create deny-set and allow-set
+	alwaysDenySet, err := set.New(conn, table, "filtered_always_denylist", nftables.TypeIPAddr)
+	if err != nil {
+		return nil, fmt.Errorf("new deny set: %w", err)
+	}
+	alwaysAllowSet, err := set.New(conn, table, "filtered_always_allowlist", nftables.TypeIPAddr)
+	if err != nil {
+		return nil, fmt.Errorf("new allow set: %w", err)
+	}
+
 	denySet, err := set.New(conn, table, "filtered_denylist", nftables.TypeIPAddr)
 	if err != nil {
 		return nil, fmt.Errorf("new deny set: %w", err)
@@ -61,14 +73,15 @@ func NewFirewall(tapIf string, hyperloopIP string) (*Firewall, error) {
 	}
 
 	fw := &Firewall{
-		conn:                 conn,
-		table:                table,
-		chain:                chain,
-		blockInternetTraffic: false,
-		denySet:              denySet,
-		allowSet:             allowSet,
-		tapInterface:         tapIf,
-		allowedRanges:        []string{fmt.Sprintf("%s/32", hyperloopIP)},
+		conn:               conn,
+		table:              table,
+		chain:              chain,
+		predefinedDenySet:  alwaysDenySet,
+		predefinedAllowSet: alwaysAllowSet,
+		userDenySet:        denySet,
+		userAllowSet:       allowSet,
+		tapInterface:       tapIf,
+		allowedRanges:      []string{fmt.Sprintf("%s/32", hyperloopIP)},
 	}
 
 	// Add firewall rules to the chain
@@ -77,9 +90,9 @@ func NewFirewall(tapIf string, hyperloopIP string) (*Firewall, error) {
 	}
 
 	// Populate the sets with initial data
-	err = fw.ResetAllCustom()
+	err = fw.ResetAllSets()
 	if err != nil {
-		return nil, fmt.Errorf("error while configuring initial deny set: %w", err)
+		return nil, fmt.Errorf("error while configuring initial data: %w", err)
 	}
 
 	return fw, nil
@@ -119,26 +132,52 @@ func (fw *Firewall) installRules() error {
 		),
 	})
 
-	// Allow anything in allowSet
+	// The order should be
+	// 1. Allow anything in predefinedAllowSet
+	// 2. Deny anything in predefinedDenySet
+	// 3. Allow anything in userAllowSet
+	// 4. Deny anything in userDenySet
+
+	// Allow anything in userAllowSet
 	fw.conn.InsertRule(&nftables.Rule{
 		Table: fw.table, Chain: fw.chain,
 		Exprs: append(ifaceMatch,
 			expressions.IPv4DestinationAddress(1),
-			expressions.IPSetLookUp(fw.allowSet.Set(), 1),
+			expressions.IPSetLookUp(fw.userAllowSet.Set(), 1),
 			expressions.Accept(),
 		),
 	})
 
-	// Drop anything in denySet
+	// Drop anything in userDenySet
 	fw.conn.AddRule(&nftables.Rule{
 		Table: fw.table, Chain: fw.chain,
 		Exprs: append(ifaceMatch,
 			expressions.IPv4DestinationAddress(1),
-			expressions.IPSetLookUp(fw.denySet.Set(), 1),
+			expressions.IPSetLookUp(fw.userDenySet.Set(), 1),
 			expressions.Drop(),
 		),
 	})
 
+	// Drop anything in predefinedDenySet
+	fw.conn.InsertRule(&nftables.Rule{
+		Table: fw.table, Chain: fw.chain,
+		Exprs: append(ifaceMatch,
+			expressions.IPv4DestinationAddress(1),
+			expressions.IPSetLookUp(fw.predefinedDenySet.Set(), 1),
+			expressions.Drop(),
+		),
+	})
+
+	// Accept anything in predefinedAllowSet
+	fw.conn.InsertRule(&nftables.Rule{
+		Table: fw.table, Chain: fw.chain,
+		Exprs: append(ifaceMatch,
+			expressions.IPv4DestinationAddress(1),
+			expressions.IPSetLookUp(fw.predefinedAllowSet.Set(), 1),
+			expressions.Accept(),
+		),
+	})
+
 	if err := fw.conn.Flush(); err != nil {
 		return fmt.Errorf("flush nftables changes: %w", err)
 	}
@@ -148,118 +187,117 @@ func (fw *Firewall) installRules() error {
 
 // AddDeniedCIDR adds a single CIDR to the deny set at runtime.
 func (fw *Firewall) AddDeniedCIDR(cidr string) error {
-	if fw.blockInternetTraffic {
-		// If internet is denied, we don't need to add any other addresses to the deny set.
-		// Because 0.0.0.0/0 is not valid IP per GoLang, we can't add new addresses to the deny set.
-		return nil
-	}
-
-	// 0.0.0.0/0 is not valid IP per GoLang, so we handle it as a special case
-	if cidr == sandbox_network.AllInternetTrafficCIDR {
-		fw.blockInternetTraffic = true
-
-		fw.conn.FlushSet(fw.denySet.Set())
-
-		toAppend := []nftables.SetElement{
-			{Key: netip.MustParseAddr("0.0.0.0").AsSlice()},
-			{
-				Key:         netip.MustParseAddr("255.255.255.255").AsSlice(),
-				IntervalEnd: true,
-			},
-		}
-
-		if err := fw.conn.SetAddElements(fw.denySet.Set(), toAppend); err != nil {
-			return fmt.Errorf("add elements to denied set: %w", err)
-		}
-	} else {
-		current, err := fw.denySet.Elements(fw.conn)
-		if err != nil {
-			return err
-		}
-
-		data, err := set.AddressStringsToSetData([]string{cidr})
-		if err != nil {
-			return err
-		}
-		merged := append(current, data...)
-		if err := fw.denySet.ClearAndAddElements(fw.conn, merged); err != nil {
-			return err
-		}
+	err := addCIDRToSet(fw.conn, fw.userDenySet, cidr)
+	if err != nil {
+		return fmt.Errorf("add denied CIDR to set: %w", err)
 	}
 
-	err := fw.conn.Flush()
+	err = fw.conn.Flush()
 	if err != nil {
-		return fmt.Errorf("flush add denied cidr changes: %w", err)
+		return fmt.Errorf("flush add denied changes: %w", err)
 	}
 
 	return nil
 }
 
 // AddAllowedCIDR adds a single CIDR to the allow set at runtime.
 func (fw *Firewall) AddAllowedCIDR(cidr string) error {
-	err := sandbox_network.CanAllowCIDR(cidr)
+	err := addCIDRToSet(fw.conn, fw.userAllowSet, cidr)
 	if err != nil {
-		return err
-	}
-
-	data, err := set.AddressStringsToSetData([]string{cidr})
-	if err != nil {
-		return err
-	}
-	current, err := fw.allowSet.Elements(fw.conn)
-	if err != nil {
-		return err
-	}
-	merged := append(current, data...)
-	if err := fw.allowSet.ClearAndAddElements(fw.conn, merged); err != nil {
-		return err
+		return fmt.Errorf("add allowed CIDR to set: %w", err)
 	}
 
 	err = fw.conn.Flush()
 	if err != nil {
-		return fmt.Errorf("flush add allowed IP changes: %w", err)
+		return fmt.Errorf("flush add allowed changes: %w", err)
 	}
 
 	return nil
 }
 
-func (fw *Firewall) ResetAllCustom() error {
-	if err := fw.ResetDeniedCustom(); err != nil {
-		return fmt.Errorf("clear block set: %w", err)
+func (fw *Firewall) ResetAllSets() error {
+	if err := fw.ResetDeniedSets(); err != nil {
+		return fmt.Errorf("clear denied set: %w", err)
 	}
-	if err := fw.ResetAllowedCustom(); err != nil {
+	if err := fw.ResetAllowedSets(); err != nil {
 		return fmt.Errorf("clear allow set: %w", err)
 	}
 
 	return nil
 }
 
-// ResetDeniedCustom resets the deny set back to original ranges.
-func (fw *Firewall) ResetDeniedCustom() error {
-	initData, err := set.AddressStringsToSetData(sandbox_network.DeniedSandboxCIDRs)
-	if err != nil {
-		return fmt.Errorf("parse initial denied CIDRs: %w", err)
+// ResetDeniedSets resets the deny set back to original ranges.
+func (fw *Firewall) ResetDeniedSets() error {
+	// Always deny the default ranges
+	if err := fw.predefinedDenySet.ClearAndAddElements(fw.conn, sandbox_network.DeniedSandboxSetData); err != nil {
+		return err
 	}
 
-	if err := fw.denySet.ClearAndAddElements(fw.conn, initData); err != nil {
+	// User defined denied ranges
+	if err := fw.userDenySet.ClearAndAddElements(fw.conn, nil); err != nil {
 		return err
 	}
 
-	fw.blockInternetTraffic = false
-
 	return fw.conn.Flush()
 }
 
-// ResetAllowedCustom resets allow set back to original ranges.
-func (fw *Firewall) ResetAllowedCustom() error {
+// ResetAllowedSets resets allow set back to original ranges.
+func (fw *Firewall) ResetAllowedSets() error {
+	// Always allowed ranges
 	initData, err := set.AddressStringsToSetData(fw.allowedRanges)
 	if err != nil {
 		return fmt.Errorf("parse initial allowed CIDRs: %w", err)
 	}
+	if err := fw.predefinedAllowSet.ClearAndAddElements(fw.conn, initData); err != nil {
+		return err
+	}
 
-	if err := fw.allowSet.ClearAndAddElements(fw.conn, initData); err != nil {
+	// User defined allowed ranges
+	if err := fw.userAllowSet.ClearAndAddElements(fw.conn, nil); err != nil {
 		return err
 	}
 
 	return fw.conn.Flush()
 }
+
+func addCIDRToSet(conn *nftables.Conn, ipset set.Set, cidr string) error {
+	current, err := ipset.Elements(conn)
+	if err != nil {
+		return err
+	}
+
+	if len(current) == 1 && current[0].AddressRangeStart == netip.MustParseAddr("0.0.0.0") && current[0].AddressRangeEnd == netip.MustParseAddr("255.255.255.255") {
+		// Because 0.0.0.0/0 is not valid IP per GoLang, we can't add new addresses to the set.
+		return nil
+	}
+
+	// 0.0.0.0/0 is not valid IP per GoLang, so we handle it as a special case
+	if cidr == sandbox_network.AllInternetTrafficCIDR {
+		conn.FlushSet(ipset.Set())
+
+		toAppend := []nftables.SetElement{
+			{
+				Key: netip.MustParseAddr("0.0.0.0").AsSlice(),
+			},
+			{
+				Key:         netip.MustParseAddr("255.255.255.255").AsSlice(),
+				IntervalEnd: true,
+			},
+		}
+
+		if err := conn.SetAddElements(ipset.Set(), toAppend); err != nil {
+			return fmt.Errorf("add elements to denied set: %w", err)
+		}
+
+		return nil
+	}
+
+	data, err := set.AddressStringsToSetData([]string{cidr})
+	if err != nil {
+		return err
+	}
+
+	merged := append(current, data...)
+
+	return ipset.ClearAndAddElements(conn, merged)
+}

packages/orchestrator/internal/sandbox/network/slot.go

@@ -309,7 +309,7 @@ func (s *Slot) ResetInternet(ctx context.Context) error {
 	defer n.Close()
 
 	err = n.Do(func(_ ns.NetNS) error {
-		err := s.Firewall.ResetAllCustom()
+		err := s.Firewall.ResetAllSets()
 		if err != nil {
 			return fmt.Errorf("error cleaning firewall rules: %w", err)
 		}