From a7c922cbd433bfbaeb315facd34b7cfd8b1fe833 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Florian=20B=C3=BCtler?= <flbuetle@ethz.ch>
Date: Wed, 17 Apr 2024 11:52:18 +0200
Subject: [PATCH] checking for extra entries in an ACL

---
 private/path/pathpol/acl.go         | 21 ++++++++++++++++++++-
 private/path/pathpol/acl_test.go    | 21 ++++++++++++++++++---
 private/path/pathpol/policy_test.go | 13 +++++++------
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/private/path/pathpol/acl.go b/private/path/pathpol/acl.go
index 9b167e6c1d..bf7d846948 100644
--- a/private/path/pathpol/acl.go
+++ b/private/path/pathpol/acl.go
@@ -27,6 +27,8 @@ import (
 var (
 	// ErrNoDefault indicates that there is no default acl entry.
 	ErrNoDefault = errors.New("ACL does not have a default")
+	// ErrExtraEntries indicates that there extra entries after the default entry.
+	ErrExtraEntries = errors.New("ACL has unused extra entries after a default entry")
 )
 
 type ACL struct {
@@ -97,9 +99,26 @@ func (a *ACL) evalInterface(iface snet.PathInterface, ingress bool) ACLAction {
 }
 
 func validateACL(entries []*ACLEntry) error {
-	if len(entries) == 0 || !entries[len(entries)-1].Rule.matchesAll() {
+	if len(entries) == 0 {
 		return ErrNoDefault
 	}
+
+	foundAt := -1
+	for i, e := range entries {
+		if e.Rule.matchesAll() {
+			foundAt = i
+			break
+		}
+	}
+
+	if foundAt < 0 {
+		return ErrNoDefault
+	}
+
+	if foundAt != len(entries)-1 {
+		return ErrExtraEntries
+	}
+
 	return nil
 }
 
diff --git a/private/path/pathpol/acl_test.go b/private/path/pathpol/acl_test.go
index e733017eae..7842afe79e 100644
--- a/private/path/pathpol/acl_test.go
+++ b/private/path/pathpol/acl_test.go
@@ -15,10 +15,12 @@
 package pathpol
 
 import (
+	"encoding/json"
 	"testing"
 
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
+	"gopkg.in/yaml.v2"
 
 	"github.com/scionproto/scion/pkg/addr"
 	"github.com/scionproto/scion/pkg/private/common"
@@ -60,7 +62,7 @@ func TestNewACL(t *testing.T) {
 	}
 }
 
-func TestUnmarshalJSON(t *testing.T) {
+func TestUnmarshal(t *testing.T) {
 	tests := map[string]struct {
 		Input       string
 		ExpectedErr error
@@ -79,11 +81,24 @@ func TestUnmarshalJSON(t *testing.T) {
 		"Entry with hop predicates": {
 			Input: `["+ 42", "-"]`,
 		},
+		"Extra entries (first)": {
+			Input:       `["-", "+ 27"]`,
+			ExpectedErr: ErrExtraEntries,
+		},
+		"Extra entries (in the middle)": {
+			Input:       `["+ 42", "-", "+ 27", "- 30"]`,
+			ExpectedErr: ErrExtraEntries,
+		},
 	}
 	for name, test := range tests {
-		t.Run(name, func(t *testing.T) {
+		t.Run("json: "+name, func(t *testing.T) {
+			var acl ACL
+			err := json.Unmarshal([]byte(test.Input), &acl)
+			assert.ErrorIs(t, err, test.ExpectedErr)
+		})
+		t.Run("yaml: "+name, func(t *testing.T) {
 			var acl ACL
-			err := acl.UnmarshalJSON([]byte(test.Input))
+			err := yaml.Unmarshal([]byte(test.Input), &acl)
 			assert.ErrorIs(t, err, test.ExpectedErr)
 		})
 	}
diff --git a/private/path/pathpol/policy_test.go b/private/path/pathpol/policy_test.go
index 9f32d887a1..60137d86e2 100644
--- a/private/path/pathpol/policy_test.go
+++ b/private/path/pathpol/policy_test.go
@@ -586,16 +586,17 @@ func TestFilterOpt(t *testing.T) {
 }
 
 func TestPolicyJsonConversion(t *testing.T) {
+	acl, err := NewACL([]*ACLEntry{
+		{Action: Allow, Rule: mustHopPredicate(t, "42-0#0")},
+		denyEntry,
+	}...)
+	require.NoError(t, err)
+
 	policy := NewPolicy("", nil, nil, []Option{
 		{
 			Policy: &ExtPolicy{
 				Policy: &Policy{
-					ACL: &ACL{
-						Entries: []*ACLEntry{
-							{Action: Allow, Rule: mustHopPredicate(t, "0-0#0")},
-							denyEntry,
-						},
-					},
+					ACL: acl,
 					LocalISDAS: &LocalISDAS{
 						AllowedIAs: []addr.IA{
 							xtest.MustParseIA("64-123"),