checking for extra entries in an ACL

scionproto · Apr 18, 2024 · a7c922c · a7c922c
1 parent e5b1f72
commit a7c922c
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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"),